Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor simple Unit Tests #7571

Merged
merged 12 commits into from
May 6, 2021
Merged

Refactor simple Unit Tests #7571

merged 12 commits into from
May 6, 2021

Conversation

Davfon
Copy link
Contributor

@Davfon Davfon commented Mar 26, 2021

I have refactored some simple unit tests to make them easier to understand, more readable, more maintainable and less likely for human error. Some identified test smells (like General Fixture and Test Code Duplication) have been fixed.
It addresses #6207, although it is mainly a refactoring.

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

It is unnecessary to create a new formatter for each test.
("test smell": General Fixture / Test Code Duplication)

renamed UnicodeConverterTest to UnicodeConverterFormatterTest
It is unnecessary to create a new formatter for each test.
("test smell": General Fixture / Test Code Duplication)
It is unnecessary to create a new formatter for each test.
("test smell": General Fixture / Test Code Duplication)
It is unnecessary to create a new comparator before each test and setting it to null after each test.
("test smell": General Fixture / Test Code Duplication)
test code quality improvement
("test smell": Test Code Duplication)
easier to maintain, less risk of error propagation.
public void tearDown() {
comparator = null;
}
private static final CrossRefEntryComparator comparator = new CrossRefEntryComparator();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change this? I think it's good practice to initialize the object under test in a setup method, so that e.g. changes to it in one test doesn't leak to other tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I see it correctly it is not possible to change the CrossRefEntryComparator Object after creation. In other words there are no setter methods and the constructor does not take any arguments.

That is why I think the beforeeach is superfluous. Why create a new object, if the one created can be used?

On the other hand, I get your point. If changes to this class are done, like adding arguments to the constructor or adding setter methods, then the BeforeEach makes sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creation of objects I java is extremely efficient. This shouldn't be a concern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In tests, one just does not use static.

  • Possiblity one: class variable (you can go for that, because the setup just does that) --> move the initialization code up.
  • Possibility two: use setup (as in the code)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reason for no static: Each test has to be independed of the last test. Thus, everything under test has to be in the initial state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! I'll revert these changes then and use the setup method for those formatter tests.

@Siedlerchr
Copy link
Member

It's nice that you are interested in contributing to JabRef, however, it would be nice if you not only focused on simple technical issues, but on issues that affect the users.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small comments, then it should be good to go.

public void tearDown() {
comparator = null;
}
private static final CrossRefEntryComparator comparator = new CrossRefEntryComparator();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In tests, one just does not use static.

  • Possiblity one: class variable (you can go for that, because the setup just does that) --> move the initialization code up.
  • Possibility two: use setup (as in the code)

assertCaseChangerTitleLowers("Hallo world- how", "HAllo WORLD- HOW");
private static Stream<Arguments> provideStringsForTitleLowers() {
return Stream.of(
// part1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please try to summarize the part to a more speaking name (or just delete the comment and use empty lines as separator between the blocks)

}

private void assertCaseChangerAllLowers(final String string, final String string2) {
assertEquals(string, BibtexCaseChanger.changeCase(string2, FORMAT_MODE.ALL_LOWERS));
/* the real test would look like as follows. Also from the comment of issue 176, order reversed as the "should be" comes first
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add @Disabled and do not work with commented code.

Arguments.of("e", "{\\'e}"),
Arguments.of("Edouard Masterly", "{\\'{E}}douard Masterly"),
Arguments.of("Ulrich Underwood and Ned Net and Paul Pot", "Ulrich {\\\"{U}}nderwood and Ned {\\~N}et and Paul {\\={P}}ot")
);
}

private void assertPurify(final String string, final String string2) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please integrate in testPurify. We don't see why this private (!) method is there.

new ProtectedTermsLoader(new ProtectedTermsPreferences(ProtectedTermsLoader.getInternalLists(),
Collections.emptyList(), Collections.emptyList(), Collections.emptyList())));
}
private static final ProtectTermsFormatter formatter = new ProtectTermsFormatter(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static final ProtectTermsFormatter formatter = new ProtectTermsFormatter(
private ProtectTermsFormatter formatter = new ProtectTermsFormatter(

public void setUp() {
formatter = new LowerCaseFormatter();
}
private static final LowerCaseFormatter formatter = new LowerCaseFormatter();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static final LowerCaseFormatter formatter = new LowerCaseFormatter();
private LowerCaseFormatter formatter = new LowerCaseFormatter();

public void setUp() {
formatter = new CapitalizeFormatter();
}
private static final CapitalizeFormatter formatter = new CapitalizeFormatter();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static final CapitalizeFormatter formatter = new CapitalizeFormatter();
private CapitalizeFormatter formatter = new CapitalizeFormatter();

public void setUp() {
formatter = new UnitsToLatexFormatter();
}
private static final UnitsToLatexFormatter formatter = new UnitsToLatexFormatter();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static final UnitsToLatexFormatter formatter = new UnitsToLatexFormatter();
private UnitsToLatexFormatter formatter = new UnitsToLatexFormatter();

void setUp() {
formatter = new UnicodeToLatexFormatter();
}
private static final UnicodeToLatexFormatter formatter = new UnicodeToLatexFormatter();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static final UnicodeToLatexFormatter formatter = new UnicodeToLatexFormatter();
private UnicodeToLatexFormatter formatter = new UnicodeToLatexFormatter();

@koppor koppor added the status: changes required Pull requests that are not yet complete label Apr 12, 2021
- remove unmeaningful comment
- testSpecialBracketPlacement
- add disabled for commented code
- integrate assertion in testPurify
@Davfon
Copy link
Contributor Author

Davfon commented Apr 13, 2021

Some small comments, then it should be good to go.

Thanks for the comments and suggested changes. I addressed them in the last commits.

@Davfon Davfon marked this pull request as ready for review April 23, 2021 07:33
@koppor koppor removed the status: changes required Pull requests that are not yet complete label May 6, 2021
Comment on lines +139 to +146
@Disabled
@Test
public void testTitleCaseAllUppers() {
/* the real test would look like as follows. Also from the comment of issue 176, order reversed as the "should be" comes first */
// assertCaseChangerTitleUppers("This is a Simple Example {TITLE}", "This is a simple example {TITLE}");
// assertCaseChangerTitleUppers("This {IS} Another Simple Example Tit{LE}", "This {IS} another simple example tit{LE}");
// assertCaseChangerTitleUppers("{What ABOUT thIS} one?", "{What ABOUT thIS} one?");
// assertCaseChangerTitleUppers("{And {thIS} might {a{lso}} be possible}", "{And {thIS} might {a{lso}} be possible}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case a test is @Disabled, the contents of the test can go enabled. Since this is probably the last comment on this PR, I'll let it go and we will fix for ourselves.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now good to go. Note that the changes of #7697 to NoBibTexFieldCheckerTest was better as that were paramterized tests.

@koppor koppor merged commit 55e7523 into JabRef:main May 6, 2021
Siedlerchr added a commit that referenced this pull request May 15, 2021
* upstream/main: (71 commits)
  [Bot] Update CSL styles (#7735)
  Fix for issue 6966: open all files of multiple entries (#7709)
  Add simple unit tests (#7696)
  Add simple unit tests (#7543)
  Update check-outdated-dependencies.yml
  Added preset for new entry keybindings and reintroduced defaults (#7705)
  Select the entry which has smaller dictonary order when merge (#7708)
  Update CHANGELOG.md
  fix: make more fields, fomatters, ids and languages sorted by alphabetical order (#7717)
  Bump libreoffice from 7.1.2 to 7.1.3 (#7721)
  Bump unoloader from 7.1.2 to 7.1.3 (#7724)
  Bump org.beryx.jlink from 2.23.7 to 2.23.8 (#7723)
  Bump org.openjfx.javafxplugin from 0.0.9 to 0.0.10 (#7725)
  fix: make fields sorted by lexicographical order (#7711)
  Fix tests
  Refactoring existing unit tests (#7687)
  Refactoring and addition of unit tests (#7581)
  Refactor simple Unit Tests (#7571)
  Add simple unit tests (#7544)
  add and extend unit tests (#7685)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants