Open
Conversation
🚨 TestLens detected 36 failed tests 🚨Here is what you can do:
Test Summary
🏷️ Commit: 4cd72a8 Test Failures (first 5 of 36)ArXivFetcherTest > findFullTextByTitleWithCurlyBracket() (:jablib:fetcherTest in Fetcher Tests / Fetcher tests)
ArXivFetcherTest > searchEntryByPartOfTitleWithAcuteAccent() (:jablib:fetcherTest in Fetcher Tests / Fetcher tests)
ArXivFetcherTest > supportsPhraseSearch() (:jablib:fetcherTest in Fetcher Tests / Fetcher tests)CompositeIdFetcherTest > performSearchByIdReturnsCorrectEntryForIdentifier(String, BibEntry, String) > 1 "performSearchByIdReturnsCorrectEntryForArXivId" (:jablib:fetcherTest in Fetcher Tests / Fetcher tests)
CrawlerTest > whetherAllFilesAreCreated() (:jablib:fetcherTest in Fetcher Tests / Fetcher tests)
Muted TestsSelect tests to mute in this pull request:
Reuse successful test results:
Click the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app. |
Member
|
Need an AI which immediatly says to me, why JabRef needs a kotlin stdlib :) |
...and with that the need to require 'kotlin.stdlib'
This is not automatically wired by the GradleX plugins that interact here.
Collaborator
Author
jjohannes
force-pushed
the
reduce-complexity-in-dependencies-setup
branch
from
bfc3604 to
987b080
Compare
jjohannes
force-pushed
the
reduce-complexity-in-dependencies-setup
branch
from
987b080 to
8898165
Compare
...plus some general build file cleanup
...to make the special cases easier to spot
Member
|
These json object thingies are due to JDK25 vs JDK26 and JavaFX. We could switch to JDK26-EA 😇 Update: OK, I saw cef941a |
jjohannes
force-pushed
the
reduce-complexity-in-dependencies-setup
branch
from
53297d9 to
589e47a
Compare
Collaborator
Author
|
@koppor do you have a clue how this can happen: Is this maybe a pipeline configuration issue that already exists and that this PR only shows because it made changes in the Or is this a known problem I can ignore? |
Member
Just move on. I bet that the jar snapshot on maven central will differ after this PR is merged. If it still fails after publishing a new snapshot, we can investigate why generated pom files behave differently than hand-crafted gradle files 😅 |
jjohannes
commented
Feb 20, 2026
| mergeJar("dev.langchain4j:langchain4j-mistral-ai") | ||
| mergeJar("dev.langchain4j:langchain4j-open-ai") | ||
| mergeJar("dev.langchain4j:langchain4j-google-ai-gemini") | ||
| requires("jtokkit") |
Collaborator
Author
There was a problem hiding this comment.
Replaced by addRuntimeOnlyDependency("com.knuddels:jtokkit") above which the requireAllDefinedDependencies() picks up,
jjohannes
commented
Feb 20, 2026
| exports("com.sun.javafx.scene") | ||
| opens("com.sun.javafx.application", "org.testfx") | ||
| opens("javafx.stage", "com.pixelduke.fxthemes") | ||
| opens("com.sun.javafx.tk.quantum", "com.pixelduke.fxthemes") |
Collaborator
Author
There was a problem hiding this comment.
I ran into some difficult to figure out situation regarding the javafx dependencies and their transitive dependencies. I wanted to limit the patching of them to the minimum to minimize confusion and clarify why patching is needed at all.
Now, all tests run and my (minimal) manual testing did not show any issues. If there is still some accessibility error at runtime now, the "opens" list here needs to be extended. But I would like to suggest to keep it explicit: use preserveExisting(), and then only add on top what is really needed.
jjohannes
commented
Feb 20, 2026
| configurations.named("${name}RuntimeClasspath") { | ||
| shouldResolveConsistentlyWith(mainRuntimeClasspath) | ||
| attributes.attributeProvider(TargetJvmVersion.TARGET_JVM_VERSION_ATTRIBUTE, jdkVersion) | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Learned something here and created follow ups to make this more concise in the future:
jjohannes
commented
Feb 20, 2026
| add("org.openjfx:jdk-jsobject") | ||
| } else { | ||
| removeIf { it.name == "jdk-jsobject" } | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
This is solving the jsobject "issue" in a way @koppor and I talked about when this came up on slack. In theory, JavaFx could publish metadata that allows Gradle to automatically make a better decission.
This rule class is adding this metadata, which allows us to remove the copy-paste solution from individual build Gradle files.
Collaborator
Author
|
This worked better than I thought. 😅 Feel free to take a look at this and test whenever you have time. I propose to add the dependency check to the pipeline, but you can also revert that part if you do not want that (yet). Now, if code is modified, the check will tell if:
To add a new dependency in a module:
|
Contributor
Review Summary by QodoActivate full java-module-dependencies plugin and consolidate dependency definitions
WalkthroughsDescription• Activate java-module-dependencies plugin to define dependencies in module-info.java • Remove redundant dependencies blocks from build.gradle.kts files • Mark runtime-only dependencies with /*runtime*/ comments in module declarations • Add dependency analysis CI pipeline step to validate module-info consistency • Refactor test module configurations using testModuleInfo DSL • Handle platform-specific JavaFX variants and JDK version-specific dependencies Diagramflowchart LR
A["module-info.java files"] -->|define all dependencies| B["java-module-dependencies plugin"]
B -->|validates consistency| C["dependency-analysis plugin"]
C -->|reports unused/missing| D["checkAllModuleInfo task"]
E["build.gradle.kts files"] -->|remove redundant blocks| F["cleaner build config"]
G["Runtime-only deps"] -->|marked with comments| A
H["Platform-specific variants"] -->|handled by metadata rules| B
File Changes1. jabgui/src/main/java/module-info.java
|
Contributor
Code Review by Qodo
1. Commented-out Lucene directives
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+166
to
+170
| // uses org.apache.lucene.codecs.lucene103.Lucene103Codec; | ||
| // requires org.apache.lucene.analysis.common; | ||
| // requires org.apache.lucene.core; | ||
| // requires org.apache.lucene.highlighter; | ||
| // requires org.apache.lucene.queryparser; |
Contributor
There was a problem hiding this comment.
1. Commented-out lucene directives 📘 Rule violation ✓ Correctness
The PR introduces commented-out uses/requires lines in module-info.java, leaving dead code in place. This reduces readability and can cause confusion about which modules are actually required.
Agent Prompt
## Issue description
Commented-out `uses`/`requires` directives were introduced in `module-info.java`, which violates the rule against commented-out code.
## Issue Context
These directives appear in the Lucene region and were previously active module declarations.
## Fix Focus Areas
- jabgui/src/main/java/module-info.java[166-170]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Collaborator
Author
There was a problem hiding this comment.
This is a bit unclear to me. I could not find where this is needed. But maybe it cannot be completely removed (?)
If it is really not needed, we can remove this completely (and not comment it out).
Contributor
There was a problem hiding this comment.
Ask❓
This is a bit unclear to me. I could not find where this is needed. But maybe it cannot be completely removed (?) If it is really not needed, we can remove this completely (and not comment it out).
Answer:
In jabgui/src/main/java/module-info.java the Lucene directives were commented out because, after enabling checkAllModuleInfo / java-module-dependencies, the analysis apparently concluded that jabgui does not directly reference Lucene types anymore (or at least not in a way the module system sees), so the directives became “unused” from the perspective of that module.
Where Lucene is needed now is primarily jablib:
jablib/src/main/java/module-info.javastill has the Lucene block, including theuses org.apache.lucene.codecs.lucene103.Lucene103Codec;and therequires ...for the Lucene modules (with some switched totransitive).- That means consumers that need Lucene “through” jablib should get the right modules via jablib’s declarations (especially where
transitiveis used).
So to your question: yes, if jabgui truly doesn’t use Lucene directly, you can remove these lines completely. Keeping them commented out is just dead code and does not help the build; it only preserves a “hint” to humans, but it makes the module descriptor less clear.
Suggested cleanup options:
- Best: remove the entire commented-out block in the Lucene region from
jabgui/module-info.java. - If you want to retain the rationale, keep a short explanatory comment only, e.g.:
// Lucene is provided via jablib; jabgui does not depend on it directly.
One thing to double-check before deleting:
- Ensure
jabguidoes not import anyorg.apache.lucene.*classes directly (IDE “Find in module”). - If it does (e.g., a UI class directly uses a QueryParser or Analyzer), then the correct fix is not commented-out lines, but re-adding the required Lucene modules in jabgui (or making the relevant Lucene requirements
transitivein jablib if the types “leak” through exported APIs).
Given the diff as-is, there’s no indication that Lucene is “special runtime-only” for jabgui; it looks simply unused there, so removing it entirely should be safe and clearer.
Comment on lines
59
to
63
| // See https://javadoc.io/doc/org.mockito/mockito-core/latest/org.mockito/org/mockito/Mockito.html#0.3 | ||
| val mockitoAgent = configurations.create("mockitoAgent") | ||
| dependencies { | ||
| mockitoAgent("org.mockito:mockito-core:5.18.0") { isTransitive = false } | ||
| } |
Contributor
There was a problem hiding this comment.
2. Mockito agent version mismatch 🐞 Bug ⛯ Reliability
The shared test convention pins the Mockito javaagent to 5.18.0, while the repo’s central constraints pin mockito-core to 5.21.0. This breaks the “single source of truth” for versions and risks running tests with an agent version that diverges from the Mockito version used elsewhere in the build.
Agent Prompt
### Issue description
`mockitoAgent` is pinned to `org.mockito:mockito-core:5.18.0` in the shared test convention plugin, but the repo’s `:versions` platform constrains Mockito to `5.21.0`. This introduces version divergence and can cause subtle issues if the agent and the Mockito library end up on different versions.
### Issue Context
The PR is explicitly trying to reduce dependency setup complexity and centralize declarations. Having a hard-coded Mockito version inside build-logic works against that goal.
### Fix Focus Areas
- build-logic/src/main/kotlin/org.jabref.gradle.feature.test.gradle.kts[59-63]
- versions/build.gradle.kts[138-142]
### Suggested fix
Option A (simplest): change `5.18.0` to `5.21.0` in the `mockitoAgent(...)` declaration.
Option B (more in spirit of the PR): remove the explicit version in `mockitoAgent("org.mockito:mockito-core")` and ensure the `mockitoAgent` configuration participates in the same version alignment mechanism used for the rest of the build (e.g., by applying the platform/constraints to that configuration).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
288
to
290
| tasks.register<Test>("fetcherTest") { | ||
| enabled = false | ||
| group = LifecycleBasePlugin.VERIFICATION_GROUP |
Contributor
There was a problem hiding this comment.
3. Fetcher tests silently skipped 🐞 Bug ⛯ Reliability
The fetcherTest Gradle test task is now explicitly disabled, but the dedicated GitHub Actions workflow still runs gradle fetcherTest. This will result in CI reporting success while running no fetcher tests, reducing coverage for a critical area.
Agent Prompt
### Issue description
The `fetcherTest` task is unconditionally disabled (`enabled = false`) in `jablib/build.gradle.kts`, but CI still runs `gradle fetcherTest`. This makes the Fetcher Tests workflow a false positive.
### Issue Context
There is a dedicated workflow `.github/workflows/tests-code-fetchers.yml` intended to validate fetcher behavior regularly and on relevant changes.
### Fix Focus Areas
- jablib/build.gradle.kts[286-297]
- .github/workflows/tests-code-fetchers.yml[48-63]
### Suggested fix
- Remove `enabled = false` so the task runs as expected.
If you need it off by default locally, gate it instead, e.g.:
- `enabled = providers.environmentVariable("CI").isPresent || providers.gradleProperty("enableFetcherTests").isPresent`
and update documentation/CI accordingly.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
jjohannes
commented
Feb 20, 2026
| api("org.glassfish.jersey.core:jersey-server") | ||
| api("org.glassfish.jersey.inject:jersey-hk2") | ||
| api("org.glassfish.jersey.test-framework:jersey-test-framework-core") | ||
| api("org.glassfish.jersey.test-framework.providers:jersey-test-framework-provider-grizzly2") |
Collaborator
Author
There was a problem hiding this comment.
This is generally not correct most of the time. If you add a dependency to a module (a JAR) here - i.e. a dependency without platform(...) – it means the dependency is used on all module paths. This is usually never what you want. For example, before this change the test dependencies listed here were also part of the runtime module path to package JabRef and likely ended up in the application package.
This file should only be used for versions. Only because a BOM is not a version on it's own, but metadata that contains versions, we have the dependencies {} block. But as a BOM does not contain a Jar (it's only metadata) it is correct to use the dependencies {} block only for BOM (platform) dependencies here.
The name is taken from module-info by default (by java-module-dependencies plugin that parses the file).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issues and pull requests
PR Description
As follow up to earlier updates on dependency management in the JabRef repo, with the goal to remove as much unnecessary complexity as possible from the dependencies setup. For that, this PR activates the full capabilities of the java-module-dependencies plugin, which is:
module-info.javafiles, effectively removing the redundantdependencies {}blocks frombuild.gradle.ktsfilescheckAllModuleInfoto check if the declarations in amodule-info.javafile are consistent with what is used (or not used) in the source code of the corresponding module.The PR removes or updates declarations depending on the advice
checkAllModuleInfogives.Steps to test
After this change, everything should build like before, but the module paths should be reduced to what is actually needed.
There is a risk that this accidentally removes a needed runtime only dependency that is not covered by tests (I'll try to be thorough). The
checkAllModuleInfotask can not know all things that are dynamically loaded at runtime. So some manual testing before integrating this would be good.Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)