Add test to JobGroupTest #675
Merged
Conversation
This file contains 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
fedejeanne
force-pushed
the
enhancements/add-testShouldCancel_5-to-JobGroupTest
branch
4 times, most recently
from
79efc2e to
430ac3b
Compare
|
I'm a bit baffled about this error (it happens under Linux): It doesn't say much. Any ideas what could it be or how to fix it? |
HeikoKlare
requested changes
Sep 13, 2023
There was a problem hiding this comment.
The added test randomly fails due to multiple race conditions. In particular, there is no order of the jobs to be executed, so cancelling the job group once a job with a specific number is executed does not give any guarantee regarding the number of jobs that have run in the end.
To reproduce a failure due to a race condition, add a sleep of several milliseconds into the if clause of the run method. The test will constantly fail.
...sts/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/JobGroupTest.java
Outdated
Show resolved
Hide resolved
...sts/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/JobGroupTest.java
Outdated
Show resolved
Hide resolved
...sts/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/JobGroupTest.java
Outdated
Show resolved
Hide resolved
...sts/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/JobGroupTest.java
Outdated
Show resolved
Hide resolved
fedejeanne
force-pushed
the
enhancements/add-testShouldCancel_5-to-JobGroupTest
branch
from
430ac3b to
6cf0bbe
Compare
fedejeanne
force-pushed
the
enhancements/add-testShouldCancel_5-to-JobGroupTest
branch
from
6cf0bbe to
baa0a8e
Compare
HeikoKlare
reviewed
Sep 20, 2023
There was a problem hiding this comment.
I propose the usage of a barrier instead of multiple latches for the reasons I give in the code comment.
In addition, the current implementation does not guarantee that all jobs are finished when the test finishes, as waitForCompletion has a timeout and may leave the jobs running. Using a barrier that waits in the main thread for at least the time the jobs themselves wait on that barrier avoids that risk.
...sts/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/JobGroupTest.java
Outdated
Show resolved
Hide resolved
fedejeanne
force-pushed
the
enhancements/add-testShouldCancel_5-to-JobGroupTest
branch
from
baa0a8e to
3b64a07
Compare
HeikoKlare
reviewed
Sep 21, 2023
...sts/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/JobGroupTest.java
Outdated
Show resolved
Hide resolved
...sts/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/JobGroupTest.java
Outdated
Show resolved
Hide resolved
...sts/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/JobGroupTest.java
Show resolved
Hide resolved
fedejeanne
force-pushed
the
enhancements/add-testShouldCancel_5-to-JobGroupTest
branch
from
e4b7455 to
97a12cf
Compare
fedejeanne
force-pushed
the
enhancements/add-testShouldCancel_5-to-JobGroupTest
branch
from
97a12cf to
a6323c1
Compare
HeikoKlare
requested changes
Sep 25, 2023
...sts/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/JobGroupTest.java
Show resolved
Hide resolved
...sts/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/JobGroupTest.java
Outdated
Show resolved
Hide resolved
fedejeanne
force-pushed
the
enhancements/add-testShouldCancel_5-to-JobGroupTest
branch
from
95acc32 to
e0b0d0f
Compare
Add the test method org.eclipse.core.tests.runtime.jobs.JobGroupTest.testShouldCancel_5(). See discussion: eclipse-platform#654
fedejeanne
force-pushed
the
enhancements/add-testShouldCancel_5-to-JobGroupTest
branch
from
e0b0d0f to
4aef675
Compare
HeikoKlare
approved these changes
Sep 26, 2023
fedejeanne
deleted the
enhancements/add-testShouldCancel_5-to-JobGroupTest
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Add the test method
org.eclipse.core.tests.runtime.jobs.JobGroupTest.testShouldCancel_5(). This test proves that the cancelation of "sibling" jobs in aJobGroupshould be done via the cancelation policy of the group and not via the monitor.This PR provides doesn't solve any issue, it merely provides context to this discussion: #654 (see comment)