Refactor simple Unit Tests by Davfon · Pull Request #7571 · JabRef/jabref

Davfon · 2021-03-26T16:38:09Z

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 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.

tobiasdiez · 2021-03-26T16:56:44Z

-    public void tearDown() {
-        comparator = null;
-    }
+    private static final CrossRefEntryComparator comparator = new CrossRefEntryComparator();


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.

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.

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

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)

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.

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

Siedlerchr · 2021-03-26T18:07:54Z

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.

koppor

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

koppor · 2021-04-12T19:03:40Z

-    public void tearDown() {
-        comparator = null;
-    }
+    private static final CrossRefEntryComparator comparator = new CrossRefEntryComparator();


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)

koppor · 2021-04-12T19:04:22Z

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


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)

koppor · 2021-04-12T19:07:02Z


-    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


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

koppor · 2021-04-12T19:07:58Z

+        );
    }

    private void assertPurify(final String string, final String string2) {


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

koppor · 2021-04-12T19:54:03Z

-                new ProtectedTermsLoader(new ProtectedTermsPreferences(ProtectedTermsLoader.getInternalLists(),
-                        Collections.emptyList(), Collections.emptyList(), Collections.emptyList())));
-    }
+    private static final ProtectTermsFormatter formatter = new ProtectTermsFormatter(


Suggested change

private static final ProtectTermsFormatter formatter = new ProtectTermsFormatter(

private ProtectTermsFormatter formatter = new ProtectTermsFormatter(

koppor · 2021-04-12T19:54:12Z

-    public void setUp() {
-        formatter = new LowerCaseFormatter();
-    }
+    private static final LowerCaseFormatter formatter = new LowerCaseFormatter();


Suggested change

private static final LowerCaseFormatter formatter = new LowerCaseFormatter();

private LowerCaseFormatter formatter = new LowerCaseFormatter();

koppor · 2021-04-12T19:54:24Z

-    public void setUp() {
-        formatter = new CapitalizeFormatter();
-    }
+    private static final CapitalizeFormatter formatter = new CapitalizeFormatter();


Suggested change

private static final CapitalizeFormatter formatter = new CapitalizeFormatter();

private CapitalizeFormatter formatter = new CapitalizeFormatter();

koppor · 2021-04-12T19:54:36Z

-    public void setUp() {
-        formatter = new UnitsToLatexFormatter();
-    }
+    private static final UnitsToLatexFormatter formatter = new UnitsToLatexFormatter();


Suggested change

private static final UnitsToLatexFormatter formatter = new UnitsToLatexFormatter();

private UnitsToLatexFormatter formatter = new UnitsToLatexFormatter();

koppor · 2021-04-12T19:54:46Z

-    void setUp() {
-        formatter = new UnicodeToLatexFormatter();
-    }
+    private static final UnicodeToLatexFormatter formatter = new UnicodeToLatexFormatter();


Suggested change

private static final UnicodeToLatexFormatter formatter = new UnicodeToLatexFormatter();

private UnicodeToLatexFormatter formatter = new UnicodeToLatexFormatter();

- remove unmeaningful comment - testSpecialBracketPlacement - add disabled for commented code - integrate assertion in testPurify

Davfon · 2021-04-13T17:22:24Z

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

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

koppor · 2021-05-06T20:51:09Z

+    @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}")


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.

koppor

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

* 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) ...

Davfon added 8 commits March 24, 2021 21:46

Increase line/statement coverage (simple unit tests)

93a97bc

Merge remote-tracking branch 'upstream/master' into a2-df

9c62748

fix typos

4d11f5f

move formatter creation out of @beforeeach

5ef25ef

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

move formatter creation out of @beforeeach

e9aa9b3

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

move formatter creation out of @beforeeach

3563a1b

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

remove unnecessary @beforeeach & @AfterEach

7e4a486

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)

convert to Parameterized Tests

98004a9

test code quality improvement ("test smell": Test Code Duplication) easier to maintain, less risk of error propagation.

tobiasdiez reviewed Mar 26, 2021

View reviewed changes

koppor requested changes Apr 12, 2021

View reviewed changes

koppor added the status: changes-required Pull requests that are not yet complete label Apr 12, 2021

Davfon added 3 commits April 13, 2021 12:11

Revert BeforeEach changes, use setup method

94d4625

BibtexCaseChangersTest & BibtexPurifyTest

50420b9

- remove unmeaningful comment - testSpecialBracketPlacement - add disabled for commented code - integrate assertion in testPurify

Fix CheckStyle

a23e989

Davfon marked this pull request as ready for review April 23, 2021 07:33

koppor removed the status: changes-required Pull requests that are not yet complete label May 6, 2021

koppor approved these changes May 6, 2021

View reviewed changes

Merge branch 'main' into a2-df

a30c0d5

koppor approved these changes May 6, 2021

View reviewed changes

koppor merged commit 55e7523 into JabRef:main May 6, 2021

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

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

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

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

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

Uh oh!

Conversation

Davfon commented Mar 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Mar 26, 2021

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Davfon commented Apr 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants