Refactor some formats tests to use parameterized tests by merlinymy · Pull Request #14558 · JabRef/jabref

merlinymy · 2025-12-09T03:58:02Z

Refs JabRef#676
I am part of a Java graduate school class. My classmates will submit other PRs that address this issue as well.

This PR refactors multiple test classes to follow the one-assertion-per-test guideline and improve overall test readability and maintainability. Parameterized tests (@CsvSource) were introduced where appropriate to replace repetitive assertions.

Steps to test
To test this code run:

Steps to test

./gradlew :jablib:test \ --tests org.jabref.logic.layout.format.AuthorAndsReplacerTest \ --tests org.jabref.logic.layout.format.AuthorFirstLastOxfordCommasTest \ --tests org.jabref.logic.layout.format.AuthorLastFirstAbbrOxfordCommasTest \ --tests org.jabref.logic.layout.format.AuthorLastFirstAbbreviatorTest \ --tests org.jabref.logic.layout.format.RemoveBracketsTest \ --tests org.jabref.logic.layout.format.RemoveLatexCommandsFormatterTest

Mandatory checks

I own the copyright of the code submitted and I license it under the MIT license
[/] I manually tested my changes in running JabRef (always required)
[/] I added JUnit tests for changes (if applicable)
[/] I added screenshots in the PR description (if change is visible to the user)
[/] I described the change in CHANGELOG.md in a way that is understandable for the average user (if change is visible to the user)
I checked the user documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request updating file(s) in https://github.com/JabRef/user-documentation/tree/main/en.

koppor · 2025-12-09T08:02:52Z

-        assertEquals("bla", LayoutEntry.parseMethodsCalls("bla,foo").getFirst().getFirst());
-        assertEquals("foo", LayoutEntry.parseMethodsCalls("bla,foo").get(1).getFirst());
+    @ParameterizedTest
+    @CsvSource(delimiterString = "->", textBlock = """


We like the default separator"," more 😅

Moreover with -> the intention "expected" and "input" is wrong.

koppor · 2025-12-09T08:04:42Z

-        assertEquals("test", LayoutEntry.parseMethodsCalls("bla(test),foo(fark)").getFirst().get(1));
-        assertEquals("fark", LayoutEntry.parseMethodsCalls("bla(test),foo(fark)").get(1).get(1));
+    @ParameterizedTest
+    @CsvSource(delimiterString = "->", textBlock = """


Here, one could use ; as delimiter

One could also use ' to sourround the string

koppor · 2025-12-09T08:05:03Z

-        assertEquals("fark", LayoutEntry.parseMethodsCalls("bla(test),foo(fark)").get(1).get(1));
+    @ParameterizedTest
+    @CsvSource(delimiterString = "->", textBlock = """
+            bla -> foo -> test -> fark -> bla(\"test\"),foo(\"fark\")


No need for " in CSV AFAIK

koppor · 2025-12-10T04:29:48Z

+    @CsvSource(textBlock = """
+            bla , bla
+            bla , bla
+            _bla.bla.blub , _bla.bla.blub
+            """)


This is more readable, but reads strange as there is a space before the comma

What about

bla, bla

? This would make it more close to "natural" sentences.

koppor · 2025-12-10T07:00:25Z

+    @ParameterizedTest
+    @CsvSource(textBlock = """
+            bla, bla
+            bla, bla


I didn't see in the first review that the second test case was `bla,'. Note the final comma! You removed it while converting. I think, it should be possible to get this in again with following change:

Suggested change

bla, bla

bla, 'bla,'

koppor · 2025-12-10T07:02:34Z

-        assertEquals("test", LayoutEntry.parseMethodsCalls("bla(test),foo(fark)").getFirst().get(1));
-        assertEquals("fark", LayoutEntry.parseMethodsCalls("bla(test),foo(fark)").get(1).get(1));
+    @ParameterizedTest
+    @CsvSource(delimiterString = ";", textBlock = """


Test caes in one package should be consistent. Below, you used , and '. Here, you use " and '. Please the remove the explicit delimiterString ";" here and change ";" to "," below. Since you use ', it should work.

koppor · 2025-12-10T23:07:17Z

-        assertEquals(2, LayoutEntry.parseMethodsCalls("bla(\"test\"),foo(\"fark\")").size());
-        assertEquals("bla", LayoutEntry.parseMethodsCalls("bla(\"test\"),foo(\"fark\")").getFirst().getFirst());
-        assertEquals("foo", LayoutEntry.parseMethodsCalls("bla(\"test\"),foo(\"fark\")").get(1).getFirst());
-        assertEquals("test", LayoutEntry.parseMethodsCalls("bla(\"test\"),foo(\"fark\")").getFirst().get(1));
-        assertEquals("fark", LayoutEntry.parseMethodsCalls("bla(\"test\"),foo(\"fark\")").get(1).get(1));
+    @Test


These whole tests seem to got lost. I don't find a line

'bla("test"),foo("fark")'

Good catch, Thank you:) It should be fixed now

@merlinymy Are you really using "Good catch" when communicating with others? Imaging you are asking a friend to review text for you and she finds something. You really say "good catch"? Does this change if you are taking an assignment? You are handing in an exam and then you talk to the teacher. Do you really say "good catch" if she recognizes a wrong answer in your exam?

This is entirely on me, not Merlin. I recommended documents (such as How To Give a Great Code Review) that advise such responses. We're in Cailfornia, where we tend to be informal and disregard hierarchy. I will advise my students in the future that it is better to err on the side of formality.

@espertusnu Thank you for clarification. Normally, only ChatGPT creates such a text. And we have very bad experiences in LLM-generated answers. Thus, I am bit biased against "Good catch" :)

koppor

I have no energy to request another change and check if the update was correct. Sometimes, new errors come with new commits.

Please, in future, recheck the diff for your self

It is obvious that @CsvSource could have been used at one more place.

koppor · 2025-12-11T09:44:04Z

+            bla, foo, test, fark, 'bla("test"),foo("fark")'
+            bla, foo, test, fark, 'bla(test),foo(fark)'
+            """)


A perfect PR would have reorded things to make if more reacble

bla, test, foo, fark

But it is OK as is.

koppor · 2025-12-11T09:58:54Z

Update: I finished the PR for myself 😅. Maybe you learn something from the diff.

* Parameterize tests (issue JabRef#676) * Address code review feedback * Fix code style * Remove spaces before delimiter in CSV test data * Restore trailing comma in test cases * Add missing test case * Adress review comments --------- Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>

Parameterize tests (issue JabRef#676)

006547d

github-actions Bot added the status: changes-required Pull requests that are not yet complete label Dec 9, 2025

koppor requested changes Dec 9, 2025

View reviewed changes

merlinymy added 2 commits December 9, 2025 13:45

Address code review feedback

64147be

Fix code style

ac85758

github-actions Bot removed the status: changes-required Pull requests that are not yet complete label Dec 10, 2025

koppor reviewed Dec 10, 2025

View reviewed changes

Remove spaces before delimiter in CSV test data

341b3ac

koppor requested changes Dec 10, 2025

View reviewed changes

github-actions Bot added the status: changes-required Pull requests that are not yet complete label Dec 10, 2025

calixtus changed the title ~~Refactor tests to use parameterized tests~~ Refactor some formats tests to use parameterized tests Dec 10, 2025

Restore trailing comma in test cases

28584f0

github-actions Bot removed the status: changes-required Pull requests that are not yet complete label Dec 10, 2025

koppor requested changes Dec 10, 2025

View reviewed changes

github-actions Bot added the status: changes-required Pull requests that are not yet complete label Dec 10, 2025

Add missing test case

167c3b1

github-actions Bot removed the status: changes-required Pull requests that are not yet complete label Dec 11, 2025

koppor previously approved these changes Dec 11, 2025

View reviewed changes

koppor added this pull request to the merge queue Dec 11, 2025

koppor removed this pull request from the merge queue due to a manual request Dec 11, 2025

Adress review comments

8ac22e9

koppor dismissed their stale review via 8ac22e9 December 11, 2025 09:58

koppor enabled auto-merge December 11, 2025 09:59

koppor approved these changes Dec 11, 2025

View reviewed changes

koppor added this pull request to the merge queue Dec 11, 2025

Merged via the queue into JabRef:main with commit b6ab730 Dec 11, 2025
48 checks passed

Uh oh!

Conversation

merlinymy commented Dec 9, 2025

Steps to test

Mandatory checks

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!

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!

koppor Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

koppor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor commented Dec 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

koppor Dec 10, 2025 •

edited

Loading

koppor left a comment •

edited

Loading