Disable JabLib tests not working on Windows#12869
Conversation
| void tearDown() { | ||
| this.indexer.closeAndWait(); | ||
| } |
There was a problem hiding this comment.
The tearDown method is unnecessary when using @tempdir, as JUnit automatically handles cleanup of temporary directories. This violates the instruction to avoid redundant cleanup.
There was a problem hiding this comment.
No. The indexer locks the directory!
|
Together with #12868, maybe we can get rid of this one?
|
Yes, there is a lot of overlap right now between the bot texts and FAQ page. |
|
The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build. |
|
I still get 31 failing tests |
|
What is the status here? |
Someone needs to go through the failing tests and decide whether they should be fixed or if it can be disabled. I plan to do it as soon as I find time. |
Pull request was converted to draft
|
Side track: These tests take really long - I don't understand why
|
|
I used
|
|
@subhramit Could you re-check. I tested "only" the "main" tests, not fetcher or database tests - and jablib only. I cannot run jabkit tests on Windows - command line too long. 🙈 |
|
|
||
| assertEquals("/home/user/test.blg;", serialized.get("blgFilePath-" + user)); | ||
| // On Windows, the path separator is a backslash, which is escaped in JabRef (see org.jabref.logic.exporter.MetaDataSerializer.serializeMetaData) | ||
| assertEquals(blgPath.toString().replace("\\", "\\\\") + ";", serialized.get("blgFilePath-" + user)); |
There was a problem hiding this comment.
The comment above the assertion is trivial as it restates the code logic. Comments should provide additional context or reasoning, not describe the code.
|
I also added separate Windows tests for the basic JabLib functionality on Windows to enable automated checking |
|
|
||
| public class DisabledOnCIServerWindowsCondition implements ExecutionCondition { | ||
|
|
||
| private static final ConditionEvaluationResult ENABLED = ConditionEvaluationResult.enabled("Running not on Windows or not on CI server"); |
There was a problem hiding this comment.
The string message should be sentence case and end with a period to maintain consistency and readability across the codebase.
This reverts commit d5fcb36.
| Map<String, String> serialized = MetaDataSerializer.getSerializedStringMap(metaData, pattern); | ||
|
|
||
| assertEquals("/home/user/test.blg;", serialized.get("blgFilePath-" + user)); | ||
| // On Windows, the path separator is a backslash, which is escaped in JabRef (see org.jabref.logic.exporter.MetaDataSerializer.serializeMetaData) |
There was a problem hiding this comment.
The comment is trivial as it restates the code logic without adding new information or reasoning. Comments should provide insights not directly derivable from the code.
| } | ||
|
|
||
| @Test | ||
| @DisabledOnOs(value = org.junit.jupiter.api.condition.OS.WINDOWS, disabledReason = "Assumed path separator is /") |
There was a problem hiding this comment.
The use of @DisabledOnOs annotation is correct, but the reason provided is trivial and does not add new information. It should provide a more meaningful explanation.
|
@trag-bot didn't find any issues in the code! ✅✨ |
With this PR, all tests in
:testshould work on WindowMandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)