Fix Random Failing UIEditWorkingSetWizardAuto.testEditPage #275

[Wittmaxi]

In my experiments, the test failed in roughly 2% of runs.

The test didn't close all processes and editors before deleting the files it contained.
This PR Makes the Job-Manager join all jobs before finally deleting the project-root.

This fix introduces a significant slowdown of the test (testing takes up to 2 seconds!) but since this is a single test and the test is important and the amount of fails (2%!) is big enough, I believe this tradeoff is very much justified.

I am confident in this fix working after seeing 400 consecutive runs that did not contain any fails (on windows).

#275

[github-actions]

Test Results

   921 files  ±0     921 suites  ±0   45m 48s ⏱️ - 2m 3s
 7 516 tests ±0   7 365 ✅ ±0  151 💤 ±0  0 ❌ ±0 
23 700 runs  ±0  23 193 ✅ ±0  507 💤 ±0  0 ❌ ±0 

Results for commit d684fab. ± Comparison against base commit e987ac1.

♻️ This comment has been updated with latest results.

[Wittmaxi]

Random failing tests for this PR are documented here:

#1583
#1005

[HeikoKlare]

Great to see this test failure being addressed, thank you! I have some questions/comments on the proposed fix.

[Wittmaxi]

In 2000 consecutive runs, the error did not occur. If you try this yourself: there is an error occurring on the second run of the test:

This fail ALWAYS happens, ALWAYS on the second run of the series and NEVER in another place - I do not believe it to be worth our time since it is almost certainly related to having multiple runs back-to-back instead of running the test once (as it is intended to be).

Additionally, the test is now much faster, taking about 0.25s on average

[Wittmaxi]

Random Failing test documented here: #1005

[Wittmaxi]

I believe the fix to be more adequate now

[fedejeanne]

You should recover the original value for auto-building after finishing the test.

[fedejeanne]

You shouldn't assume true was the original value.

Instead, save the original value in doSetUp and apply it again in doTearDown

[HeikoKlare]

Since there will be other tests that do not clean up properly, it is usually better to restore defaults than restoring the state before the test. So a general cleanup (also used by the platform resource tests), is to just restore the default workspace description:

ResourcesPlugin.getWorkspace().setDescription(Workspace.defaultWorkspaceDescription());

[fedejeanne]

This would make the test(s) meddle with each other but I guess since one is restoring the default value then it should be OK.

In that case, why not push this all the way to the top to UITestCase::tearDown?

[HeikoKlare]

This would make the test(s) meddle with each other

It would do the exact opposite.

In that case, why not push this all the way to the top to UITestCase::tearDown?

Because it is out-of-scope of this PR to refactor the complete UI test infrastructure. Having unified cleanups to ensure proper test isolation would of course make sense but should be done (1) as a separate task and (2) should be done properly using mechanisms like JUnit 4 rules or JUnit 5 extensions instead of fostering test class inheritance.

[fedejeanne]

This would make the test(s) meddle with each other

It would do the exact opposite.

It's a hack that makes this test "fix whatever other tests might have broken", that's what I meant. But it's ok, it's outside the scope of this PR to hunt down all the offending tests ;-) . BTW do you already know of any such tests (by name) or does this have to be investigated yet?

[HeikoKlare]

BTW do you already know of any such tests (by name) or does this have to be investigated yet?

If I knew them, I would have created a ticket or fixed them. Most of the test failures I faced in the last year have one of two reasons:

1. Race conditions
2. Improper test isolation

What can be done systematically to ensure isolation is to encapsulate state changes or potential leaks (like job execution) into proper rules (JUnit 4) or extensions (JUnit 5).

[Wittmaxi]

I have now changed the Setup/Teardown to

1. disable autobuilding for the test
2. reset the workspace description (as Heiko suggested)

@Override
  public void doSetUp() throws Exception {
  super.doSetUp();
  setAutoBuilding(false);
}

@Override
public void doTearDown() throws Exception {
  super.doTearDown();
  ResourcesPlugin.getWorkspace().setDescription(Workspace.defaultWorkspaceDescription());
}

[HeikoKlare]

Failing checks because of new issues are only because of an outdated reference build. Recent master build reports the same new issues: https://ci.eclipse.org/platform/job/eclipse.platform.ui/job/master/421/