Conversation
subhramit
added
[outdated] type: documentation
component: libre-office
dev: code-quality
| assertEquals(expected, citation); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The comment is IMHO helpful. For someone diving into it 1 year later.
There was a problem hiding this comment.
Marked disabled and kept the comment.
|
@Siedlerchr @koppor @ThiloteE Review changes are done. |
|
Also manually adapted as per upcoming best practices enforcement: #11643. |
| String expected = "[Smit2016]"; | ||
|
|
||
| assertEquals(expected, inTextCitationText); |
There was a problem hiding this comment.
One-liners can be inlined
| String expected = "[Smit2016]"; | |
| assertEquals(expected, inTextCitationText); | |
| assertEquals("[Smit2016]", inTextCitationText); |
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("citationStyleProvider") |
There was a problem hiding this comment.
Typically, name the providing method the same as the testing method
@MethodSourceHowever, in your case, you are providing one source for multiple tests.
Please split up your test case in three:
Name the methods as follows:
citationStylePresenttitleMatchesnumericPropertyMatches
Do NOT use explanations for assertEquals. All of your text can be read out of the code.
Maybe, you are searching for the name parameter of @ParamterizedTest (https://stackoverflow.com/a/57894201/873282)?
There was a problem hiding this comment.
Maybe, you are searching for the name parameter of @ParamterizedTest (https://stackoverflow.com/a/57894201/873282)?
This was an interesting read. Right now, we don't need it but can come in handy sometime.
There was a problem hiding this comment.
Updated: used same name for provider and test method uniformly throughout PR.
| /** | ||
| * Test to check transformation of raw, unsupported HTML into OO-ready HTML. | ||
| * | ||
| * @param input the raw HTML. |
There was a problem hiding this comment.
Rename parameter input to rawHtml and remove comment. Thus, the parameter name is self-explanatory.
| static Stream<Arguments> rawHTMLProvider() { | ||
| return Stream.of( | ||
|
|
||
| // First three are general test cases for unescaping HTML entities |
There was a problem hiding this comment.
Use // region: ... and then // endregion. Reason: Someone might add new test cases and might not update the commet.
| * <b>Precondition:</b> This test assumes that {@link CitationStyleGenerator#generateCitation(List, String, CitationStyleOutputFormat, BibDatabaseContext, BibEntryTypesManager) generateCitation} works as expected. | ||
| * </p> | ||
| * | ||
| * @param style the CSL style to test transformation on. |
| */ | ||
| @ParameterizedTest | ||
| @MethodSource("rawBibliographyProvider") | ||
| void ooHTMLTransformFromRawBibliography(CitationStyle style, String expected) { |
There was a problem hiding this comment.
To be in line with assertEquals, the expected parameter should be passed first.
There was a problem hiding this comment.
So should this be flipped in every method (along with the provider arguments)?
There was a problem hiding this comment.
Okay, changed uniformly on all methods and providers.
| * <b>Precondition:</b> This test assumes that {@link CitationStyleGenerator#generateInText(List, String, CitationStyleOutputFormat, BibDatabaseContext, BibEntryTypesManager) generateInText} works as expected. | ||
| * </p> | ||
| * | ||
| * @param style the CSL style to test transformation on. |
|
Updated as per latest review - 3962402 |
|
In general, you don't need extensive javadoc for test classes. Javadoc is more important for the classes under test that contain the real functionality |
|
The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build. |
koppor
left a comment
koppor
left a comment
There was a problem hiding this comment.
Scrolled through. Looks good. Since Chris seemed to check thoroughly, good to go.
5 participants
Test suite, refactoring and documentation for OO-CSL components
[PR - D of the GSoC '24 CSL4LibreOffice Project]
Follow-up to #11477, #11521, #11577 and #11604
CSLFormatUtils).isNumericStyleimplemented during the project).Mandatory checks
Change inCHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)Screenshots added in PR description (for UI changes)