Skip to content
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

random failing ServiceSupplierTestCase.testOptionalReferences #488

Closed
jukzi opened this issue Jun 9, 2023 · 12 comments · Fixed by #513 or #895
Closed

random failing ServiceSupplierTestCase.testOptionalReferences #488

jukzi opened this issue Jun 9, 2023 · 12 comments · Fixed by #513 or #895
Labels
help wanted Extra attention is needed test junit test related things

Comments

@jukzi
Copy link
Contributor

jukzi commented Jun 9, 2023

on win

java.lang.AssertionError: expected:<1> but was:<2>
	at org.eclipse.e4.core.internal.tests.di.extensions.ServiceSupplierTestCase.testOptionalReferences(ServiceSupplierTestCase.java:267)
@HeikoKlare
Copy link
Contributor

HeikoKlare added a commit to HeikoKlare/eclipse.platform that referenced this issue Jun 22, 2023
…ipse-platform#488

The test case ServiceSupplierTestCase.testOptionalReferences randomly
fails because it waits for asynchronous service registration using
fixed-time sleep operations, which fail on slow machines.

This change replaces the sleep operations with functionality that waits
for changes of the service states. A timeout ensures that in case of a
failure the wait operation is aborted.
HeikoKlare added a commit to HeikoKlare/eclipse.platform that referenced this issue Jun 23, 2023
…ipse-platform#488

The test case ServiceSupplierTestCase.testOptionalReferences randomly
fails because it waits for asynchronous service registration using
fixed-time sleep operations, which fail on slow machines.

This change replaces the sleep operations with functionality that waits
for changes of the service states. A timeout ensures that in case of a
failure the wait operation is aborted.
HeikoKlare added a commit to HeikoKlare/eclipse.platform that referenced this issue Jun 27, 2023
…ipse-platform#488

The test case ServiceSupplierTestCase.testOptionalReferences randomly
fails because it waits for asynchronous service registration using
fixed-time sleep operations, which fail on slow machines.

This change replaces the sleep operations with functionality that waits
for changes of the service states. A timeout ensures that in case of a
failure the wait operation is aborted.
HeikoKlare added a commit that referenced this issue Jun 27, 2023
The test case ServiceSupplierTestCase.testOptionalReferences randomly
fails because it waits for asynchronous service registration using
fixed-time sleep operations, which fail on slow machines.

This change replaces the sleep operations with functionality that waits
for changes of the service states. A timeout ensures that in case of a
failure the wait operation is aborted.
@HeikoKlare
Copy link
Contributor

Test failed again with a different error message in recent build
https://github.com/eclipse-platform/eclipse.platform/pull/595/checks?check_run_id=15421937193

java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:87)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertNotNull(Assert.java:713)
	at org.junit.Assert.assertNotNull(Assert.java:723)
	at org.eclipse.e4.core.internal.tests.di.extensions.ServiceSupplierTestCase.testOptionalReferences(ServiceSupplierTestCase.java:254)

@Michael5601
Copy link
Contributor

Failed again in this run in PR: #605

HeikoKlare added a commit to HeikoKlare/eclipse.platform that referenced this issue Nov 6, 2023
…ipse-platform#488

The ServiceSupplierTestCase.testOptionalReferences test case randomly
fails. The test aims to wait for services to be enabled or disabled, but
it only waits until a single value is changed although it asserts that
multiple values are changed afterwards. This is a race condition, as the
order in which the values are changed or in which the changes are
visible is non-deterministic, also due to missing memory barriers.

This change addresses this issue via two means:
1. it makes the fields of the asserted values volatile to ensure that
changed values eventually become visible to the UI thread
2. it waits for all values to be changed before asserting their values
to allow different orders of value changes
HeikoKlare added a commit to HeikoKlare/eclipse.platform that referenced this issue Nov 6, 2023
…ipse-platform#488

The ServiceSupplierTestCase.testOptionalReferences test case randomly
fails. The test aims to wait for services to be enabled or disabled, but
it only waits until a single value is changed although it asserts that
multiple values are changed afterwards. This is a race condition, as the
order in which the values are changed or in which the changes are
visible is non-deterministic, also due to missing memory barriers.

This change addresses this issue via two means:
1. it makes the fields of the asserted values volatile to ensure that
changed values eventually become visible to the UI thread
2. it waits for all values to be changed before asserting their values
to allow different orders of value changes
HeikoKlare added a commit that referenced this issue Nov 6, 2023
The ServiceSupplierTestCase.testOptionalReferences test case randomly
fails. The test aims to wait for services to be enabled or disabled, but
it only waits until a single value is changed although it asserts that
multiple values are changed afterwards. This is a race condition, as the
order in which the values are changed or in which the changes are
visible is non-deterministic, also due to missing memory barriers.

This change addresses this issue via two means:
1. it makes the fields of the asserted values volatile to ensure that
changed values eventually become visible to the UI thread
2. it waits for all values to be changed before asserting their values
to allow different orders of value changes
@jukzi jukzi added the test junit test related things label Nov 28, 2023
@fedejeanne
Copy link
Contributor

Failed again in this build of #776

org.eclipse.e4.core.internal.tests.di.extensions.ServiceSupplierTestCase.testOptionalReferences -- Time elapsed: 0.070 s <<< FAILURE!
java.lang.AssertionError: expected same:<class org.eclipse.e4.core.internal.tests.di.extensions.DisabledServiceA> was not:<class org.eclipse.e4.core.internal.tests.di.extensions.DisabledServiceB>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotSame(Assert.java:829)
	at org.junit.Assert.assertSame(Assert.java:772)
	at org.junit.Assert.assertSame(Assert.java:783)
	at org.eclipse.e4.core.internal.tests.di.extensions.ServiceSupplierTestCase.testOptionalReferences(ServiceSupplierTestCase.java:270)

@jukzi jukzi added the help wanted Extra attention is needed label Nov 29, 2023
@HeikoKlare
Copy link
Contributor

It is a different kind of failure after #829. I'll have a look.

@jukzi
Copy link
Contributor Author

jukzi commented Nov 29, 2023

i can not reproduce it locally

@HeikoKlare
Copy link
Contributor

It's a race condition and thus random failing.

@HeikoKlare
Copy link
Contributor

Potential reason: the tests have been duplicated (for javax and jakarta, see #791) and I only applied #829 to one them.

@jukzi
Copy link
Contributor Author

jukzi commented Nov 29, 2023

please change the classname of the new test to avoid confusion

@HeikoKlare
Copy link
Contributor

In general, we would have to change the names/packages of all the duplicated code to ensure that we can identify in which of the implementation a problem arises. But we may again run into the problem that only one of the implementations is touched by maintenance. So from a maintenance perspective, this duplication is quite problematic.

@jukzi
Copy link
Contributor Author

jukzi commented Nov 29, 2023

in stacktraces the package name is visible, while in some junit results it's not obvious

@HeikoKlare
Copy link
Contributor

I see. One thing that can currently help is to look at the log and see which project was processed (which may also not be easy due to parallel build).

Concerning this specific issue the log shows that the error happens in the javax copy of the tests, which is the one that has not been corrected in #829. I will provide a PR to fix that.

HeikoKlare added a commit to HeikoKlare/eclipse.platform that referenced this issue Nov 29, 2023
…ipse-platform#488

Fixes two issues:
1. The ServiceSupplierTestCase asserts several values that are assigned
by an asynchronous task. To this end, it waits for the conditions to be
fulfilled by the asynchronous task. However, the conditions are
incomplete, leading to a race condition.
2. The ServiceSupplierTestCase has been duplicated for javax and jakarta
and a fix for a random failure has only been applied to a single
implementation in
eclipse-platform#829

This change corrects the conditions the test waits for and aligns both
copies of the test class with each other.

Fixes eclipse-platform#488
HeikoKlare added a commit that referenced this issue Nov 29, 2023
Fixes two issues:
1. The ServiceSupplierTestCase asserts several values that are assigned
by an asynchronous task. To this end, it waits for the conditions to be
fulfilled by the asynchronous task. However, the conditions are
incomplete, leading to a race condition.
2. The ServiceSupplierTestCase has been duplicated for javax and jakarta
and a fix for a random failure has only been applied to a single
implementation in
#829

This change corrects the conditions the test waits for and aligns both
copies of the test class with each other.

Fixes #488
Michael5601 pushed a commit to CodeLtDave/eclipse.platform that referenced this issue Feb 12, 2024
…ipse-platform#488

The ServiceSupplierTestCase.testOptionalReferences test case randomly
fails. The test aims to wait for services to be enabled or disabled, but
it only waits until a single value is changed although it asserts that
multiple values are changed afterwards. This is a race condition, as the
order in which the values are changed or in which the changes are
visible is non-deterministic, also due to missing memory barriers.

This change addresses this issue via two means:
1. it makes the fields of the asserted values volatile to ensure that
changed values eventually become visible to the UI thread
2. it waits for all values to be changed before asserting their values
to allow different orders of value changes
Michael5601 pushed a commit to CodeLtDave/eclipse.platform that referenced this issue Feb 12, 2024
…ipse-platform#488

Fixes two issues:
1. The ServiceSupplierTestCase asserts several values that are assigned
by an asynchronous task. To this end, it waits for the conditions to be
fulfilled by the asynchronous task. However, the conditions are
incomplete, leading to a race condition.
2. The ServiceSupplierTestCase has been duplicated for javax and jakarta
and a fix for a random failure has only been applied to a single
implementation in
eclipse-platform#829

This change corrects the conditions the test waits for and aligns both
copies of the test class with each other.

Fixes eclipse-platform#488
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed test junit test related things
Projects
None yet
4 participants