-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix search test NPE #11749
Fix search test NPE #11749
Conversation
@@ -315,6 +315,11 @@ public void close() { | |||
HeadlessExecutorService.INSTANCE.execute(this::closeIndex); | |||
} | |||
|
|||
@Override | |||
public void closeAndWait() { | |||
HeadlessExecutorService.INSTANCE.executeAndWait(this::closeIndex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we pass this dependency in the constructor as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check this commit 81a30ff
(#11749)? added a boolean parameter to the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sorry, I did not clarfiy. I meant the Executor Service, that would partially also improve the test siutation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR: Refactor DefaultLinkedFilesIndexer
NOT to depend on HeadlessExecutorService.INSTANCE
, but to have an ExecutorService
passed using the constructor.
--> follow-up PR
The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build. |
@@ -315,6 +315,11 @@ public void close() { | |||
HeadlessExecutorService.INSTANCE.execute(this::closeIndex); | |||
} | |||
|
|||
@Override | |||
public void closeAndWait() { | |||
HeadlessExecutorService.INSTANCE.executeAndWait(this::closeIndex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR: Refactor DefaultLinkedFilesIndexer
NOT to depend on HeadlessExecutorService.INSTANCE
, but to have an ExecutorService
passed using the constructor.
--> follow-up PR
Search tests sometimes throw NullPointerException (example: https://github.com/JabRef/jabref/actions/runs/10759644260/job/29836573460).
This issue occurs because multiple tests for the same library share the same index path. The index is closed in a background thread, which may lead to subsequent tests accessing the same index before the previous test closing index finishes.
Although the
org.jabref.gui.util.CurrentThreadTaskExecutor
is used to run background tasks sequentially for tests, a different task executor (org.jabref.logic.util.HeadlessExecutorService#INSTANCE
) is used for closing the index (due to LoayGhreeb#6).@TempDir
for the index path.closeAndWait
to wait until the task is finished. Using@TempDir
only didn't resolve the issue because the next test could start, and@TempDir
would be removed before the closing task finished.Follow-up to #11542.
Closes https://github.com/JabRef/jabref-issue-melting-pot/issues/527.
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if applicable)