Refactor tests to use parameterized tests by rgrace9 · Pull Request #14054 · JabRef/jabref

rgrace9 · 2025-10-08T21:27:43Z

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 and @MethodSource) were introduced where appropriate to replace repetitive assertions, and one test file was renamed so that it matched the case of its class name.

Steps to test

To test this code run:

./gradlew :jablib:test \
  --tests org.jabref.logic.bibtex.comparator.BibtexStringComparatorTest \
  --tests org.jabref.logic.bst.util.BstNameFormatterTest \
  --tests org.jabref.logic.crawler.StudyRepositoryTest \
  --tests org.jabref.logic.formatter.bibtexfields.HtmlToLatexFormatterTest \
  --tests org.jabref.logic.formatter.bibtexfields.LatexCleanupFormatterTest \
  --tests org.jabref.logic.formatter.bibtexfields.NormalizeISSNTest \
  --tests org.jabref.logic.formatter.bibtexfields.NormalizeNamesFormatterTest \
  --tests org.jabref.logic.formatter.bibtexfields.OrdinalsToSuperscriptFormatterTest \

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.

calixtus

Some nitpicks.

calixtus · 2025-10-14T12:24:59Z

+    private final BibtexString bs1 = new BibtexString("VLSI", "Very Large Scale Integration");
+    private final BibtexString bs2 = new BibtexString("DSP", "Digital Signal Processing");
+    private final BibtexString bs3 = new BibtexString("DSP", "Digital Signal Processing");
+    private final BibtexString bs4 = new BibtexString("DSPVLSI", "#VLSI# #DSP#");


Can you give the variables more meaningful names? Helps a lot in reading the single tests.

calixtus · 2025-10-14T12:28:41Z

+    @CsvSource({
+            // Return the same string
+            "abc, abc", "aaa, aaa", "(p < 0.01), (p < 0.01)",

-    @Test
-    void basic() {
-        assertEquals("aaa", formatter.format("aaa"));
-    }
+            // IEEE-style HTML entity for em dash
+            "Towards situation-aware adaptive workflows: SitOPT --- A general purpose situation-aware workflow management system, Towards situation-aware adaptive workflows: SitOPT &amp;#x2014; A general purpose situation-aware workflow management system",


Also here, one item per line.

Thanks for pointing that out! I updated the other arguments in this method so each test case is on its own line.

This one is a single test case, so I kept it on one line even though it’s a bit long, but I’m happy to break it up if you’d prefer.

…ine helper method, and improve readability

rgrace9 · 2025-10-14T20:44:52Z

Thank you for the code review and helpful suggestions! I’ve applied the requested changes and also rebased with the latest version of main.

After pushing, I realized I had accidentally committed local auto-generated files (resources/csl-styles, resources/csl-locales, and abbrv.jabref.org). I’ve since removed them. I now see that my git push -f has triggered the no-force-push check error — apologies for that.

Please let me know if there’s anything else you’d like me to adjust.

calixtus · 2025-10-15T15:29:47Z

looks good to me. shall we wait for your classmates to open their prs or should move forward?

calixtus · 2025-10-15T15:30:03Z

sorry, hit the wrong butten

jabref-machine · 2025-10-15T15:30:43Z

Your pull request needs to link an issue correctly.

To ease organizational workflows, please link this pull request to the issue by including a supported keyword in the pull request's description as per syntax described in GitHub's documentation.

Examples

✅ Fixes #xyz links pull-request to issue. Merging the PR will close the issue.
✅ Fixes https://github.com/JabRef/jabref/issues/xyz links pull-request to issue. Merging the PR will close the issue.
✅ Fixes https://github.com/Koppor/jabref/issues/xyz links pull-request to issue. Merging the PR will close the issue.
❌ Fixes [#xyz](https://github.com/JabRef/jabref/issues/xyz) links pull-request to issue. Merging the PR will NOT close the issue.

calixtus · 2025-10-15T16:06:10Z

The PR can always be reviewed in the merged/closed PR list. But better to have it integrated instead of keeping it in the list of open PRs for too long.

Thanks for your contribution.

calixtus · 2025-10-15T16:07:45Z

On second thought, better to wait, i can see in the issue that not everybody is done with his/her PR.

Siedlerchr · 2025-10-15T16:34:51Z

maybe we can reopen the issue after merge?

espertusnu · 2025-10-15T23:28:17Z

looks good to me. shall we wait for your classmates to open their prs or should move forward?

@koppor said by email to me (9/21) that it is okay to have separate PRs for each student (11 of them). I have instructed them to follow each other's reviews so you don't have to repeat the same guidance.

I can let you know when all are done and the issue can be closed.

Thanks for doing the reviews!

calixtus · 2025-10-16T11:58:43Z

Changed Reference in PR description from fixes to refs. Will enable merge queue again.
Thanks for the contribution and good luck with your studies!

* Parameterize tests for Issue JabRef#676 * Address PR review feedback: reformat CsvSource, rename variables, inline helper method, and improve readability

rgrace9 changed the title ~~Parameterize tests for Issue #676~~ Issue 676 - Parameterize tests Oct 8, 2025

rgrace9 changed the title ~~Issue 676 - Parameterize tests~~ Parameterize tests Oct 8, 2025

rgrace9 changed the title ~~Parameterize tests~~ Parameterize tests - Issue676 Oct 8, 2025

rgrace9 changed the title ~~Parameterize tests - Issue676~~ Refactor tests to use parameterized tests Oct 8, 2025

JabRef deleted a comment from jabref-machine Oct 11, 2025

calixtus requested changes Oct 14, 2025

View reviewed changes

Siedlerchr added the status: changes-required Pull requests that are not yet complete label Oct 14, 2025

Parameterize tests for Issue JabRef#676

a3ae25d

rgrace9 force-pushed the contribution-to-issue676 branch from 1c300be to 45d3ab0 Compare October 14, 2025 20:30

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

Address PR review feedback: reformat CsvSource, rename variables, inl…

61f1680

…ine helper method, and improve readability

rgrace9 force-pushed the contribution-to-issue676 branch from 45d3ab0 to 61f1680 Compare October 14, 2025 20:32

rgrace9 requested a review from calixtus October 14, 2025 20:56

calixtus reviewed Oct 15, 2025

View reviewed changes

Comment thread jablib/src/test/java/org/jabref/logic/crawler/StudyRepositoryTest.java

calixtus approved these changes Oct 15, 2025

View reviewed changes

calixtus closed this Oct 15, 2025

calixtus reopened this Oct 15, 2025

Siedlerchr approved these changes Oct 15, 2025

View reviewed changes

calixtus added this pull request to the merge queue Oct 15, 2025

calixtus removed this pull request from the merge queue due to a manual request Oct 15, 2025

calixtus added this pull request to the merge queue Oct 16, 2025

Merged via the queue into JabRef:main with commit bb36ce1 Oct 16, 2025
110 of 113 checks passed

Uh oh!

Conversation

rgrace9 commented Oct 8, 2025 • edited by calixtus Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Steps to test

Mandatory checks

Uh oh!

calixtus left a comment

Choose a reason for hiding this comment

Uh oh!

calixtus Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

calixtus Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

rgrace9 Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rgrace9 commented Oct 14, 2025

Uh oh!

Uh oh!

calixtus commented Oct 15, 2025

Uh oh!

calixtus commented Oct 15, 2025

Uh oh!

jabref-machine commented Oct 15, 2025

Examples

Uh oh!

calixtus commented Oct 15, 2025

Uh oh!

Uh oh!

calixtus commented Oct 15, 2025

Uh oh!

Siedlerchr commented Oct 15, 2025

Uh oh!

espertusnu commented Oct 15, 2025

Uh oh!

calixtus commented Oct 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rgrace9 commented Oct 8, 2025 •

edited by calixtus

Loading