More test stability fixes #120
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
15 tasks
jonahgraham
force-pushed
the
more_test_stability
branch
from
90a4c68
to
ed726f1
Compare
|
Getting close here - both runs on Jenkins have 0 test errors. |
jonahgraham
force-pushed
the
more_test_stability
branch
from
0974263
to
241fd24
Compare
This comment was marked as outdated.
This comment was marked as outdated.
jonahgraham
force-pushed
the
more_test_stability
branch
from
241fd24
to
b5bdf61
Compare
This comment was marked as outdated.
This comment was marked as outdated.
jonahgraham
force-pushed
the
more_test_stability
branch
from
b5bdf61
to
78d1aaa
Compare
|
I appreciate the work you are putting in to fix the tests 👍 |
I know this looks worse - but it is because I added additional checks to make sure tests were not leaving files in the workspace. Many tests aren't deleting projects. |
jonahgraham
force-pushed
the
more_test_stability
branch
12 times, most recently
from
053681d
to
6d64b5e
Compare
jonahgraham
force-pushed
the
more_test_stability
branch
from
a25e68e
to
e665cb8
Compare
This was
linked to
issues
Nov 5, 2022
jonahgraham
force-pushed
the
more_test_stability
branch
3 times, most recently
from
7d923f6
to
644b353
Compare
The resource helper is widely used, but when it deletes projects, it leaves their contents on disk. Fix this so that the contents on disk is deleted. Part of eclipse-cdt#117
On GHA the error does not really give a good indication of what went wrong with minimal to no stack traces Part of eclipse-cdt#117
Ideally the code itself should also be deleted from CDT, but this test is super flaky and I cannot seem to convert it to JUnit5 so I can properly mark it as flaky. Therefore the test is now simply gone. Part of eclipse-cdt#117
Some of these tests left behind projects, by chaning them to extend BaseTestCase5 the resource cleanup happens and the tests are cleaned up properly Part of eclipse-cdt#117
This code seems to be trying to optimize across tests. This change isolated each individual test better. Also removed is a bunch of effectively unused test code. Part of eclipse-cdt#117
This is an ancient (2004) test that does not apply, was never referenced in the Suites and whose name doesn't match standard pattern. It tried to import ancient versions of projects as well, which would kick off the project converter UI that can't be disabled from the core plug-in. All in all, this test adds nothing of value. Part of eclipse-cdt#117
JDT thinks this is a test and will run it in the IDE and display an error. But it is only used to compose other tests, by making it abstract the IDE won't see it anymore. Part of eclipse-cdt#117
And by "new" I mean 15 years old. Part of eclipse-cdt#117
While it may be that the tests don't directly rely on JUnit5, the IDE requires JUnit5 in the classpath or else the launch config doesn't work with this error: Cannot find class 'org.junit.platform.commons.annotation.Testable' on project build path. Part of eclipse-cdt#117
Make sure not to lose stack trace when a test fails. Part of eclipse-cdt#117
Also stop catching exceptions and discarding stacktraces Part of eclipse-cdt#117
The test "abused" the JUnit3 ability to control test order and required all tests to run in the given order to work. Refactored the test to remove all redundant try/catch and provide a more traditional flow. Part of eclipse-cdt#117
Instead of using assertTrue, but assertEquals we get nice error messages that show the contents of the collections. Using assertEquals(List.of() instead of assertArrayEquals(... because that gives nicer error messages. Part of eclipse-cdt#117
Rather than update this test, simply delete this test of ancient functionality Part of eclipse-cdt#117
This provides the shared pre/post condition assertions to help make sure tests are running in a clean environment. If I had time I would convert all the tests to BaseTestCase5 which is JUnit5, but I only did the tests that had something wrong with them. Part of eclipse-cdt#117
This helps add some isolation between tests in case background threads are accessing a project. However I am not sure this solves any of the actual outstanding flaky tests. Part of eclipse-cdt#117
These tests all had the same mistake, they had 5 x waitForEvent with the comment "at this point all 5 threads should be stopped" but there are actually 6 threads (5 spawned ones + main thread). Most of the time all 6 threads would be stopped in time, but sometimes one of them wouldn't be stopped, leading to a "CoreException: Context is running" error in the test This fixes the ThreadStackFrameSyncTest item Part of eclipse-cdt#117
The format of this error message used to look like: ```java Expected number (0) of Non-OK status objects in log differs from actual (1). Error while parsing /projC_testTripleDownwardV/h3.h. java.lang.reflect.InvocationTargetException Caused by: java.lang.reflect.InvocationTargetException Caused by: java.lang.AssertionError: Need to hold a write lock to clear result caches ``` and it was hard to impossible to identify what the cause is. The hope is that capturing this fuller stack trace into the log will make it easier to identify the problem. Part of eclipse-cdt#117
jonahgraham
force-pushed
the
more_test_stability
branch
from
adfe2c5
to
3e63144
Compare
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.
More work on #117