Enhance consistency check tests #13788

Jenish-1235 · 2025-08-29T21:04:59Z

Closes #13709

Added clearer asserts, resource null-checks, CLI return-code checks and safe System.out.
Enabled ArgumentProcessorTest in jabkit again.

Steps to test

To run only this test class: ./gradlew :jabkit:test --tests "org.jabref.cli.ArgumentProcessorTest"
To run the whole module tests: ./gradlew :jabkit:test

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.

…rocessorTest.java

…gain

jabkit/src/test/java/org/jabref/cli/ArgumentProcessorTest.java

calixtus · 2025-08-30T07:30:14Z

jabkit/src/test/java/org/jabref/cli/ArgumentProcessorTest.java

+        URL originUrl = ArgumentProcessorTest.class.getResource("origin.bib");
+        assertNotNull(originUrl, "Test resource origin.bib not found on classpath");
+        String fullBib = Path.of(originUrl.toURI()).toAbsolutePath().toString();
+
+        URL auxUrl = ArgumentProcessorTest.class.getResource("paper.aux");
+        assertNotNull(auxUrl, "Test resource paper.aux not found on classpath");


I believe this is not correct.
Test should assert if auxImport works and not if Path.of or getResource works (the are covered by the jdk test suite) or if the resources are there.
If the test fails, because the test resources are not there, this should be "more dramatic" than "just" to assert if the logic does not work. This is a different scope and a different level of abstraction.

Also can you confirm about auxImport test ? I am looking for solution to put the @BeforeAll annotation to checkTestResources() and remove per-test assertNotNull calls from auxImport test and it will be performed by class-level check. Am I thinking in right direction ?

calixtus · 2025-08-30T07:31:57Z

Just a quick thought in between things

Jenish-1235 · 2025-08-30T07:44:59Z

Sure, I'll fix it. Thanks

koppor · 2025-08-30T09:02:01Z

@Jenish-1235 Some small comment on the commit message - a commit message should be able to form a corerct sentence in case following is put BEFORE the message: "If applied, this commit will".

Update: Link: https://www.gitkraken.com/learn/git/best-practices/git-commit-message#using-imperative-verb-form

Jenish-1235 · 2025-08-30T09:09:03Z

@Jenish-1235 Some small comment on the commit message - a commit message should be able to form a corerct sentence in case following is put BEFORE the message: "If applied, this commit will".

Update: Link: https://www.gitkraken.com/learn/git/best-practices/git-commit-message#using-imperative-verb-form

Sure, I'll take care of this as well.

…cy-check-running-again

…e all to check for resources

…tps://github.com/Jenish-1235/jabref into task-get-test-of-consistency-check-running-again

…nning-again

…gain

jabkit/src/test/java/org/jabref/cli/ArgumentProcessorTest.java

…gain

jabkit/src/test/java/org/jabref/cli/ArgumentProcessorTest.java

koppor · 2025-09-02T21:00:25Z

jabkit/src/test/java/org/jabref/cli/ArgumentProcessorTest.java

+            // Validate the produced bib can be parsed and contains at least one entry
+            BibtexImporter importer = new BibtexImporter(importFormatPreferences, new DummyFileUpdateMonitor());
+            List<BibEntry> entries = importer.importDatabase(outputBib).getDatabase().getEntries();
+            assertTrue(entries != null && !entries.isEmpty(), "Expected output bib to contain at least one entry");


Typically, we match List.of(entry, ...) to compare the contents. I can relate that this is not necessary here.

You can keep the idea assertFalse(entries.isEmpty() -- the non-null is implicitly checked.

We typically also do not have assertion text, but since AI always generates it, it is OK to keep.

While, I wrote the initial code as you mentioned as per typical style and upon using AI tools for code review, this changes were suggested. I will be open to learn the nuances of both, and make changes if needed.

Being a new contributor, I wish to learn if this test-style is better in coding practices as entries won't be empty right ?

assertFalse(entries.isEmpty(), "Expected output bib to contain at least one entry");

koppor · 2025-09-02T21:01:26Z

jabkit/src/test/java/org/jabref/cli/ArgumentProcessorTest.java

-                Objects.requireNonNull(ArgumentProcessorTest.class.getResource("ArgumentProcessorTestExportMatches.bib"))
-                       .toURI()
-        );
+                                   Objects.requireNonNull(ArgumentProcessorTest.class.getResource("ArgumentProcessorTestExportMatches.bib"))


There is also the test at the beginning - not sure why this check is added here again.

…nning-again

…ck-running-again' into task-get-test-of-consistency-check-running-again

…gain

…ck-running-again' into task-get-test-of-consistency-check-running-again

jabkit/src/test/java/org/jabref/cli/ArgumentProcessorTest.java

…gain

…ck-running-again' into task-get-test-of-consistency-check-running-again

…gain

trag-bot · 2025-09-04T22:58:21Z

@trag-bot didn't find any issues in the code! ✅✨

subhramit · 2025-09-04T23:03:56Z

@Jenish-1235 you don't need to merge main so often on Github. If you see here on GitHub that there is a conflict, that's when you should resolve and merge as soon as feasible.

Jenish-1235 · 2025-09-04T23:07:08Z

Alright will follow that. While as a practice I was trying to keep the branch updated.

subhramit · 2025-09-05T00:28:16Z

Alright will follow that. While as a practice I was trying to keep the branch updated.

That is a good practice, but the people subscribed to the PR will keep receiving emails.
Better way - You can do that on your local with git pull upstream main - that won't create noise here on GitHub. When you are also done with your changes, you can directly push.

Jenish-1235 · 2025-09-05T01:09:42Z

Ahh got it , thanks for guiding 😄

koppor · 2025-09-08T20:28:07Z

jabkit/src/test/java/org/jabref/cli/ArgumentProcessorTest.java

-    void auxImport(@TempDir Path tempDir) throws URISyntaxException {
-        String fullBib = Path.of(ArgumentProcessorTest.class.getResource("origin.bib").toURI()).toAbsolutePath().toString();
-        String auxFile = Path.of(ArgumentProcessorTest.class.getResource("paper.aux").toURI()).toAbsolutePath().toString();
+    void auxImport(@TempDir Path tempDir) throws IOException {
+        InputStream originIs = ArgumentProcessorTest.class.getResourceAsStream("origin.bib");
+        InputStream auxIs = ArgumentProcessorTest.class.getResourceAsStream("paper.aux");


Why this change? The tests worked bevor, didn't they?

koppor · 2025-09-08T20:28:34Z

jabkit/src/test/java/org/jabref/cli/ArgumentProcessorTest.java

-        assertTrue(Files.exists(outputBib));
+        BibtexImporter importer = new BibtexImporter(importFormatPreferences, new DummyFileUpdateMonitor());
+        List<BibEntry> entries = importer.importDatabase(outputBib).getDatabase().getEntries();
+        assertTrue(entries != null && !entries.isEmpty(), "Expected output bib to contain at least one entry");


Do assertNotEquals(List.of(), entries);; -- null check is included in assertNotEquals.

IntelliJ will output the diff better in case of an error.

koppor · 2025-09-08T20:29:34Z

jabkit/src/test/java/org/jabref/cli/ArgumentProcessorTest.java

-                Objects.requireNonNull(ArgumentProcessorTest.class.getResource("ArgumentProcessorTestExportMatches.bib"))
-                       .toURI()
-        );
+                                   Objects.requireNonNull(ArgumentProcessorTest.class.getResource("ArgumentProcessorTestExportMatches.bib"))
+                                          .toURI());


Also remove requireNonNull - to have the tests as clean as the other ones.

calixtus · 2025-09-08T20:36:10Z

jabkit/src/test/java/org/jabref/cli/ArgumentProcessorTest.java

+    @ParameterizedTest(name = "Resource {0} is available on classpath")
+    @CsvSource({"origin.bib", "paper.aux", "ArgumentProcessorTestExportMatches.bib"})
+    void checkTestResourcesParam(String resource) {
+        assertNotNull(ArgumentProcessorTest.class.getResource(resource), "Required test resource missing from classpath: " + resource);
+    }


Hi, FYI: We had some discussion in the devcal about this smoke test.
We came to the conclusion, that usually these tests are not neccessary.
It came down to that: JUnit is for testing code, not your build system: Using JUnit to check whether the resources are placed correctly is redundant as the build tool is already designed to manage that.
And Classpath/resource loading failures will show up anyway: If a test relies on a missing resource, it will throw an exception and fail.
So this is not really a unit test, but probably more of an integration test.
But, as we are always paranoid about our test setup, we will leave it in. 😄

Jenish-1235 and others added 2 commits August 30, 2025 02:25

added assert messages and enabled consistency check test in ArgumentP…

356a2bc

…rocessorTest.java

Merge branch 'main' into task-get-test-of-consistency-check-running-a…

52217d7

…gain

calixtus reviewed Aug 30, 2025

View reviewed changes

Jenish-1235 and others added 5 commits August 30, 2025 14:52

Merge remote-tracking branch 'origin' into task-get-test-of-consisten…

6b40edb

…cy-check-running-again

enable more robust auxImport test and enable checkTestResources befor…

2712f4b

…e all to check for resources

Merge branch 'task-get-test-of-consistency-check-running-again' of ht…

975fea4

…tps://github.com/Jenish-1235/jabref into task-get-test-of-consistency-check-running-again

Merge branch 'JabRef:main' into task-get-test-of-consistency-check-ru…

cffee73

…nning-again

Merge branch 'main' into task-get-test-of-consistency-check-running-a…

d9cbc95

…gain

calixtus reviewed Sep 1, 2025

View reviewed changes

jabkit/src/test/java/org/jabref/cli/ArgumentProcessorTest.java Outdated Show resolved Hide resolved

Jenish-1235 added 2 commits September 2, 2025 03:15

Merge branch 'main' into task-get-test-of-consistency-check-running-a…

f28d7b5

…gain

Merge branch 'main' into task-get-test-of-consistency-check-running-a…

8bd4ff0

…gain

Jenish-1235 changed the title ~~added assert messages and enabled consistency check test in ArgumentP…~~ add assert messages and enable consistency check test in ArgumentP… Sep 2, 2025

replace IllegalStateException with IOException in checkTestResources

52c6ba4

Jenish-1235 force-pushed the task-get-test-of-consistency-check-running-again branch from fa0c7a0 to 52c6ba4 Compare September 2, 2025 21:13

trag-bot bot reviewed Sep 2, 2025

View reviewed changes

jabkit/src/test/java/org/jabref/cli/ArgumentProcessorTest.java Outdated Show resolved Hide resolved

Jenish-1235 added 5 commits September 3, 2025 02:52

remove redundant test resources exsistence check

4aa43f7

fix csl-styles submodule changes

8e2dae4

fix csl-locales submodule changes

0a8a87d

fix abbrv.jabref.org submodule changes

1f544d9

remove redundant comments

c30fa2b

koppor requested changes Sep 2, 2025

View reviewed changes

Jenish-1235 and others added 5 commits September 3, 2025 21:51

Merge branch 'JabRef:main' into task-get-test-of-consistency-check-ru…

6dd1b83

…nning-again

change checkTestResources test from BeforeAll to ParameterizedTest

b00baec

Merge remote-tracking branch 'origin/task-get-test-of-consistency-che…

da813ef

…ck-running-again' into task-get-test-of-consistency-check-running-again

Merge branch 'main' into task-get-test-of-consistency-check-running-a…

fe8b256

…gain

Merge remote-tracking branch 'origin/task-get-test-of-consistency-che…

6ff559e

…ck-running-again' into task-get-test-of-consistency-check-running-again

fix import checkstyles

ef4209f

Jenish-1235 requested a review from koppor September 3, 2025 19:00

trag-bot bot reviewed Sep 3, 2025

View reviewed changes

jabkit/src/test/java/org/jabref/cli/ArgumentProcessorTest.java Show resolved Hide resolved

calixtus changed the title ~~add assert messages and enable consistency check test in ArgumentP…~~ Enhance consistency check tests Sep 3, 2025

Merge branch 'main' into task-get-test-of-consistency-check-running-a…

1a20187

…gain

maorethians mentioned this pull request Sep 4, 2025

False Positive in Matching Moves tsantalis/RefactoringMiner#996

Closed

Jenish-1235 and others added 4 commits September 5, 2025 00:43

Merge branch 'main' into task-get-test-of-consistency-check-running-a…

1897db2

…gain

fix import checkstyles

bd58b14

Merge remote-tracking branch 'origin/task-get-test-of-consistency-che…

14da9c1

…ck-running-again' into task-get-test-of-consistency-check-running-again

Merge branch 'main' into task-get-test-of-consistency-check-running-a…

1ac04cb

…gain

koppor requested changes Sep 8, 2025

View reviewed changes

calixtus reviewed Sep 8, 2025

View reviewed changes

Uh oh!

Enhance consistency check tests #13788

Are you sure you want to change the base?

Enhance consistency check tests #13788

Conversation

Jenish-1235 commented Aug 29, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calixtus commented Aug 30, 2025

Uh oh!

Jenish-1235 commented Aug 30, 2025

Uh oh!

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

Uh oh!

Uh oh!

Jenish-1235 commented Aug 30, 2025

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

trag-bot bot commented Sep 4, 2025

Uh oh!

subhramit commented Sep 4, 2025

Uh oh!

Jenish-1235 commented Sep 4, 2025

Uh oh!

subhramit commented Sep 5, 2025

Uh oh!

Jenish-1235 commented Sep 5, 2025

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!

Uh oh!

Jenish-1235 commented Aug 29, 2025 •

edited by calixtus

Loading

koppor commented Aug 30, 2025 •

edited

Loading