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.

Sign up for GitHub

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

Jump to bottom

Random failing of TestBug297635 #460

Closed
fedejeanne opened this issue May 17, 2023 · 2 comments · Fixed by #465 or #737
Closed

Random failing of TestBug297635 #460

fedejeanne opened this issue May 17, 2023 · 2 comments · Fixed by #465 or #737

Comments

@fedejeanne
Copy link
Contributor

The test TestBug297635.test2 fails randomly.

One such failures can be seen in https://github.com/eclipse-platform/eclipse.platform/actions/runs/4991112285/jobs/8937471805.

Here's the stack trace:

junit.framework.AssertionFailedError
	at junit.framework.Assert.fail(Assert.java:55)
	at junit.framework.Assert.assertTrue(Assert.java:22)
	at junit.framework.Assert.assertNotNull(Assert.java:256)
	at junit.framework.Assert.assertNotNull(Assert.java:248)
	at junit.framework.TestCase.assertNotNull(TestCase.java:391)
	at org.eclipse.core.tests.resources.session.TestBug297635.assertStateTrees(TestBug297635.java:169)
	at org.eclipse.core.tests.resources.session.TestBug297635.test2(TestBug297635.java:131)
...
fedejeanne added a commit to fedejeanne/eclipse.platform that referenced this issue May 17, 2023
@fedejeanne
Fix random failing of TestBug297635 eclipse-platform#460
8218756 
Merge the 2 test methods in the class into 1 since they depend on each
other anyway. Extract private methods to improve readability.
fedejeanne added a commit to fedejeanne/eclipse.platform that referenced this issue May 17, 2023
@fedejeanne
Fix random failing of TestBug297635 eclipse-platform#460
8ae8af9 
Merge both test methods in the class into 1 since they depende on each
other anyway. Extract private methods to improve readability.
@fedejeanne fedejeanne mentioned this issue May 26, 2023
Merged
fedejeanne added a commit to fedejeanne/eclipse.platform that referenced this issue May 31, 2023
@fedejeanne
Fix random failing of TestBug297635 eclipse-platform#460
4813db4 
Merge both test methods in the class into 1 since they depend on each
other anyway. Extract private methods to improve readability. Rethrow
exceptions (they let the test fail anyway).
@fedejeanne fedejeanne mentioned this issue Jun 9, 2023
Closed
@jukzi jukzi mentioned this issue Jun 9, 2023
Merged
fedejeanne added a commit to fedejeanne/eclipse.platform that referenced this issue Jun 20, 2023
@fedejeanne
Fix random failing of TestBug297635 eclipse-platform#460
c08de7a 
Merge both test methods in the class into 1 and let the class extend
directly from ResourceTest. Extract private methods to improve
readability. Rethrow
exceptions (they let the test fail anyway).
fedejeanne added a commit to fedejeanne/eclipse.platform that referenced this issue Jun 21, 2023
@fedejeanne
Fix random failing of TestBug297635 eclipse-platform#460
5fd81b6 
Merge both test methods in the class into 1 and let the class extend
directly from ResourceTest. Extract private methods to improve
readability. Rethrow
exceptions (they let the test fail anyway).
fedejeanne added a commit to fedejeanne/eclipse.platform that referenced this issue Jun 21, 2023
@fedejeanne
Fix random failing of TestBug297635 eclipse-platform#460
0347088 
Merge both test methods in the class into 1 and let the class extend
directly from ResourceTest. Extract private methods to improve
readability. Rethrow
exceptions (they let the test fail anyway).
HeikoKlare pushed a commit that referenced this issue Jun 21, 2023
@fedejeanne @HeikoKlare
Fix random failing of TestBug297635 #460
0d516c9 
Merge both test methods in the class into 1 and let the class extend
directly from ResourceTest. Extract private methods to improve
readability. Rethrow
exceptions (they let the test fail anyway).
@HeikoKlare
Copy link
Contributor

The failing test was obviously not (properly) fixed by #465, as it still fails randomly:
https://github.com/eclipse-platform/eclipse.platform/actions/runs/5369959755/jobs/9742278023

@HeikoKlare HeikoKlare reopened this Jun 27, 2023
fedejeanne added a commit to fedejeanne/eclipse.platform that referenced this issue Aug 4, 2023
@fedejeanne
Disable the automatic snapshots for TestBug297635
1bbfce0 
Disable the automatic snapshots while running tests in TestBug297635 to
avoid random failures.

Fixes eclipse-platform#460
@fedejeanne fedejeanne mentioned this issue Aug 4, 2023
Closed
fedejeanne added a commit to fedejeanne/eclipse.platform that referenced this issue Aug 4, 2023
@fedejeanne
Disable the automatic snapshots for TestBug297635
97869ab 
Disable the automatic snapshots while running tests in TestBug297635 to
avoid random failures.

Fixes eclipse-platform#460
fedejeanne added a commit to fedejeanne/eclipse.platform that referenced this issue Aug 4, 2023
@fedejeanne
Disable the automatic snapshots for TestBug297635
a6c7df4 
Disable the automatic snapshots while running tests in TestBug297635 to
avoid random failures.

Fixes eclipse-platform#460
fedejeanne added a commit to fedejeanne/eclipse.platform that referenced this issue Aug 5, 2023
@fedejeanne
Disable the automatic snapshots for TestBug297635
ac37216 
Disable the automatic snapshots while running tests in TestBug297635 to
avoid random failures.

Fixes eclipse-platform#460
fedejeanne added a commit to fedejeanne/eclipse.platform that referenced this issue Aug 7, 2023
@fedejeanne
Disable the automatic snapshots for TestBug297635
efa6b8b 
Disable the automatic snapshots while running tests in TestBug297635 to
avoid random failures.

Fixes eclipse-platform#460
fedejeanne added a commit to fedejeanne/eclipse.platform that referenced this issue Aug 8, 2023
@fedejeanne
Disable the automatic snapshots for TestBug297635
1b35a80 
Disable the automatic snapshots while running tests in TestBug297635 to
avoid random failures.

Fixes eclipse-platform#460
@HeikoKlare
Copy link
Contributor

In my opinion, the real issue with this test is that is in an overall bad state. It validates internal states using reflection, even though that is not what the test is supposed to test. What the test is actually supposed to do (according to the bug it is a regression test for) is to ensure that saving snapshots clears the save state trees (by calling forgetSavedTree). So actually the test should check whether that method is properly called when saving snapshots rather than whether some internal state changed. Whether or not there has been some state stored before saving a snapshot or not does not matter for the test. The forgetSavedTree method is responsible for doing that (no matter how, when or why it was called).

So in my opinion a proper test for the bug would look much easier:

  • Create a mock/spy workspace
  • Create a SaveManager for that workspace
  • Make a snapshot save on the SaveManager instance
  • Validate that forgetSavedTree was called on the workspace

Everything else (whether internal states are properly cleared by the forgetSavedTree call and the like) should not be part of the test and is verified separately.

HeikoKlare added a commit to HeikoKlare/eclipse.platform that referenced this issue Oct 9, 2023
@HeikoKlare
Fix test for save state cleanup on snapshot save / bug 297635 eclipse…
2e109ce 
…-platform#460

The existing test TestBug297635 relies on reflection to test some
internal state change of the SavedState class that saves temporary
states until some save operation. The test was prone to fail because it
relied on internal state changes that depend an specific overall system
state (e.g., have an unsaved workspace state, so that no concurrent
automatic snapshot save is allowed to occur during test execution). It
used reflection to access an internal, highly volatile state.

The bug for which the test case serves as a regression test was due to
missing cleanup triggered by SavedState.forgetSavedTree(). Instead of
checking for internal state changes performed by the cleanup, the
rewritten test only checks for a call of the according method. To this
end, it temporarily inserts a spy on the SaveManager. Since a Workspace
and SaveManager are not easy to set up in an isolated way for testing
purposes, the test still relies on reflection, but only to inject a spy
on the SaveManager rather than to validate internal states.

Since the test is not required to be run as a session test anymore, it
is moved to the ordinary regression resource tests.

Fixes eclipse-platform#460.
@HeikoKlare HeikoKlare mentioned this issue Oct 9, 2023
Merged
HeikoKlare added a commit to HeikoKlare/eclipse.platform that referenced this issue Oct 9, 2023
@HeikoKlare
Fix test for save state cleanup on snapshot save / bug 297635 eclipse…
e3edb34 
…-platform#460

The existing test TestBug297635 relies on reflection to test some
internal state change of the SavedState class that saves temporary
states until some save operation. The test was prone to fail because it
relied on internal state changes that depend an specific overall system
state (e.g., have an unsaved workspace state, so that no concurrent
automatic snapshot save is allowed to occur during test execution). It
used reflection to access an internal, highly volatile state.

The bug for which the test case serves as a regression test was due to
missing cleanup triggered by SavedState.forgetSavedTree(). Instead of
checking for internal state changes performed by the cleanup, the
rewritten test only checks for a call of the according method. To this
end, it temporarily inserts a spy on the SaveManager. Since a Workspace
and SaveManager are not easy to set up in an isolated way for testing
purposes, the test still relies on reflection, but only to inject a spy
on the SaveManager rather than to validate internal states.

Since the test is not required to be run as a session test anymore, it
is moved to the ordinary regression resource tests.

Fixes eclipse-platform#460.
HeikoKlare added a commit to HeikoKlare/eclipse.platform that referenced this issue Oct 13, 2023
@HeikoKlare
Fix test for save state cleanup on snapshot save / bug 297635 eclipse…
f3abe34 
…-platform#460

The existing test TestBug297635 relies on reflection to test some
internal state change of the SavedState class that saves temporary
states until some save operation. The test was prone to fail because it
relied on internal state changes that depend an specific overall system
state (e.g., have an unsaved workspace state, so that no concurrent
automatic snapshot save is allowed to occur during test execution). It
used reflection to access an internal, highly volatile state.

The bug for which the test case serves as a regression test was due to
missing cleanup triggered by SavedState.forgetSavedTree(). Instead of
checking for internal state changes performed by the cleanup, the
rewritten test only checks for a call of the according method. To this
end, it temporarily inserts a spy on the SaveManager. Since a Workspace
and SaveManager are not easy to set up in an isolated way for testing
purposes, the test still relies on reflection, but only to inject a spy
on the SaveManager rather than to validate internal states.

Since the test is not required to be run as a session test anymore, it
is moved to the ordinary regression resource tests.

Fixes eclipse-platform#460.
HeikoKlare added a commit to HeikoKlare/eclipse.platform that referenced this issue Oct 13, 2023
@HeikoKlare
Fix test for save state cleanup on snapshot save / bug 297635 eclipse…
69978f2 
…-platform#460

The existing test TestBug297635 relies on reflection to test some
internal state change of the SavedState class that saves temporary
states until some save operation. The test was prone to fail because it
relied on internal state changes that depend an specific overall system
state (e.g., have an unsaved workspace state, so that no concurrent
automatic snapshot save is allowed to occur during test execution). It
used reflection to access an internal, highly volatile state.

The bug for which the test case serves as a regression test was due to
missing cleanup triggered by SavedState.forgetSavedTree(). Instead of
checking for internal state changes performed by the cleanup, the
rewritten test only checks for a call of the according method. To this
end, it temporarily inserts a spy on the SaveManager. Since a Workspace
and SaveManager are not easy to set up in an isolated way for testing
purposes, the test still relies on reflection, but only to inject a spy
on the SaveManager rather than to validate internal states.

Since the test is not required to be run as a session test anymore, it
is moved to the ordinary regression resource tests.

Fixes eclipse-platform#460.
HeikoKlare added a commit to HeikoKlare/eclipse.platform that referenced this issue Oct 13, 2023
@HeikoKlare
Fix test for save state cleanup on snapshot save / bug 297635 eclipse…
f9a5b76 
…-platform#460

The existing test TestBug297635 relies on reflection to test some
internal state change of the SavedState class that saves temporary
states until some save operation. The test was prone to fail because it
relied on internal state changes that depend an specific overall system
state (e.g., have an unsaved workspace state, so that no concurrent
automatic snapshot save is allowed to occur during test execution). It
used reflection to access an internal, highly volatile state.

The bug for which the test case serves as a regression test was due to
missing cleanup triggered by SavedState.forgetSavedTree(). Instead of
checking for internal state changes performed by the cleanup, the
rewritten test only checks for a call of the according method. To this
end, it temporarily inserts a spy on the SaveManager. Since a Workspace
and SaveManager are not easy to set up in an isolated way for testing
purposes, the test still relies on reflection, but only to inject a spy
on the SaveManager rather than to validate internal states.

Since the test is not required to be run as a session test anymore, it
is moved to the ordinary regression resource tests.

Fixes eclipse-platform#460.
HeikoKlare added a commit to HeikoKlare/eclipse.platform that referenced this issue Oct 13, 2023
@HeikoKlare
Fix test for save state cleanup on snapshot save / bug 297635 eclipse…
19dbe22 
…-platform#460

The existing test TestBug297635 relies on reflection to test some
internal state change of the SavedState class that saves temporary
states until some save operation. The test was prone to fail because it
relied on internal state changes that depend an specific overall system
state (e.g., have an unsaved workspace state, so that no concurrent
automatic snapshot save is allowed to occur during test execution). It
used reflection to access an internal, highly volatile state.

The bug for which the test case serves as a regression test was due to
missing cleanup triggered by SavedState.forgetSavedTree(). Instead of
checking for internal state changes performed by the cleanup, the
rewritten test only checks for a call of the according method. To this
end, it temporarily inserts a spy on the SaveManager. Since a Workspace
and SaveManager are not easy to set up in an isolated way for testing
purposes, the test still relies on reflection, but only to inject a spy
on the SaveManager rather than to validate internal states.

Since the test is not required to be run as a session test anymore, it
is moved to the ordinary regression resource tests.

Fixes eclipse-platform#460.
HeikoKlare added a commit to HeikoKlare/eclipse.platform that referenced this issue Oct 13, 2023
@HeikoKlare
Fix test for save state cleanup on snapshot save / bug 297635 eclipse…
cd75c85 
…-platform#460

The existing test TestBug297635 relies on reflection to test some
internal state change of the SavedState class that saves temporary
states until some save operation. The test was prone to fail because it
relied on internal state changes that depend an specific overall system
state (e.g., have an unsaved workspace state, so that no concurrent
automatic snapshot save is allowed to occur during test execution). It
used reflection to access an internal, highly volatile state.

The bug for which the test case serves as a regression test was due to
missing cleanup triggered by SavedState.forgetSavedTree(). Instead of
checking for internal state changes performed by the cleanup, the
rewritten test only checks for a call of the according method. To this
end, it temporarily inserts a spy on the SaveManager. Since a Workspace
and SaveManager are not easy to set up in an isolated way for testing
purposes, the test still relies on reflection, but only to inject a spy
on the SaveManager rather than to validate internal states.

Since the test is not required to be run as a session test anymore, it
is moved to the ordinary regression resource tests.

Fixes eclipse-platform#460.
HeikoKlare added a commit to HeikoKlare/eclipse.platform that referenced this issue Oct 13, 2023
@HeikoKlare
Fix test for save state cleanup on snapshot save / bug 297635 eclipse…
7afdb42 
…-platform#460

The existing test TestBug297635 relies on reflection to test some
internal state change of the SavedState class that saves temporary
states until some save operation. The test was prone to fail because it
relied on internal state changes that depend an specific overall system
state (e.g., have an unsaved workspace state, so that no concurrent
automatic snapshot save is allowed to occur during test execution). It
used reflection to access an internal, highly volatile state.

The bug for which the test case serves as a regression test was due to
missing cleanup triggered by SavedState.forgetSavedTree(). Instead of
checking for internal state changes performed by the cleanup, the
rewritten test only checks for a call of the according method. To this
end, it temporarily inserts a spy on the SaveManager. Since a Workspace
and SaveManager are not easy to set up in an isolated way for testing
purposes, the test still relies on reflection, but only to inject a spy
on the SaveManager rather than to validate internal states.

Since the test is not required to be run as a session test anymore, it
is moved to the ordinary regression resource tests.

Fixes eclipse-platform#460.
HeikoKlare added a commit that referenced this issue Oct 13, 2023
@HeikoKlare
Fix test for save state cleanup on snapshot save / bug 297635 #460
cfb3c5f 
The existing test TestBug297635 relies on reflection to test some
internal state change of the SavedState class that saves temporary
states until some save operation. The test was prone to fail because it
relied on internal state changes that depend an specific overall system
state (e.g., have an unsaved workspace state, so that no concurrent
automatic snapshot save is allowed to occur during test execution). It
used reflection to access an internal, highly volatile state.

The bug for which the test case serves as a regression test was due to
missing cleanup triggered by SavedState.forgetSavedTree(). Instead of
checking for internal state changes performed by the cleanup, the
rewritten test only checks for a call of the according method. To this
end, it temporarily inserts a spy on the SaveManager. Since a Workspace
and SaveManager are not easy to set up in an isolated way for testing
purposes, the test still relies on reflection, but only to inject a spy
on the SaveManager rather than to validate internal states.

Since the test is not required to be run as a session test anymore, it
is moved to the ordinary regression resource tests.

Fixes #460.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@HeikoKlare @fedejeanne