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

Handle flaky UI tests by waiting for the JDT index #3320

Merged
merged 20 commits into from
Jan 28, 2025

Conversation

LorenzoBettini
Copy link
Contributor

Closes #3319
Closes #3065

I added the wait for the JDT index when we do an incremental/full/clean build.
In my fork, in GitHub Actions, this removed the flakyness altogether; I also removed the Maven option to rerun tests.

Currently, the waitForJdtIndex is public but we can make it private; otherwise, I'll add the @since annotation.
I think it's better to have it public because I added an explicit call in a test that reads the bytecode: it used to fail in a flaky way due to a NPE when visiting the bytecode; maybe the .class file was created but not yet filled.

I've also added a utility monitor to log the progress on the console. If you think this could be useful, I'll add the comments and @since annotation.

In my understanding, JDT added some asynchronicity in the index, and if we don't wait for that explictly, we lose JDT information that makes our UI tests, based on building, flaky.

@LorenzoBettini
Copy link
Contributor Author

By the way, I cannot exclude there'll be further flakes. In my first experiments, new flakes came out, and I found that those needed the waiting for the JDT index as well.

Copy link

github-actions bot commented Jan 21, 2025

Test Results

  6 461 files  ±  0    6 461 suites  ±0   3h 19m 8s ⏱️ - 12m 31s
 43 241 tests ±  0   42 657 ✅ ±  0    584 💤 ±0  0 ❌ ±0 
170 051 runs   - 195  167 714 ✅  - 190  2 337 💤 ±0  0 ❌  - 5 

Results for commit 12fba88. ± Comparison against base commit 07b7598.

♻️ This comment has been updated with latest results.

@cdietrich
Copy link
Contributor

On ci test seems to have timed out. Have restarted them

@LorenzoBettini
Copy link
Contributor Author

There's still a failing test as well..

@LorenzoBettini
Copy link
Contributor Author

On ci test seems to have timed out. Have restarted them

Which one? A specific test?

the project creation and workspace cleanup are performed by the base
class.
@cdietrich
Copy link
Contributor

@LorenzoBettini
Copy link
Contributor Author

This one is still flaky though: https://github.com/LorenzoBettini/xtext/actions/runs/12903165007/job/35980864838

I don't know what's going on in that test or how it is supposed to work...

@cdietrich
Copy link
Contributor

it does

  • compile a xtend file.
  • xtends debug source installing compilation participant will hook into jdt compiler
  • test asserts the participant has added info to bytecode

@LorenzoBettini
Copy link
Contributor Author

it does

* compile a xtend file.

* xtends debug source installing compilation participant will hook into jdt compiler

* test asserts the participant has added info to bytecode

Then when it fails because there's no debug information, but the .class file has been correctly created, it means that our participant is not taken into consideration, is it? This attachment to the jdt compiler as a participant must happen asynchronously I guess, and we should find a way to synchronize with that.

@LorenzoBettini
Copy link
Contributor Author

Shall I investigate further or are we OK to merge?

@cdietrich
Copy link
Contributor

i did not find time to review your stuff yet and wont find soon

Copy link
Contributor

@szarnekow szarnekow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth an attempt and merge it.

@LorenzoBettini
Copy link
Contributor Author

@szarnekow some updates to this PR:

  • I force JDT to register its participant by triggering compilation of a Java source file 2f39318
  • I made the waitForJdtIndex private for the moment, and documented the new utility logging monitor e9a35f9
  • I converted the test from Xtend to Java for easier debugging, using Java 17 text blocks

@LorenzoBettini
Copy link
Contributor Author

Still flaky tests, but I can see that

  • UniqueClassNameValidatorUITest does not use waitForBuild and calls build directly, so waitForJdtIndex is not used there, and sometimes it fails
  • This Storage2UriMapperJdtImplTest has to be inspected accordingly
  • This Storage2UriMapperJavaImplTest sometimes throws:
java.util.ConcurrentModificationException
	at java.base/java.util.LinkedHashMap$LinkedHashIterator.nextNode(LinkedHashMap.java:1023)
	at java.base/java.util.LinkedHashMap$LinkedValueIterator.next(LinkedHashMap.java:1052)
	at org.eclipse.xtext.ui.resource.Storage2UriMapperJavaImpl.getStorages(Storage2UriMapperJavaImpl.java:393)
	at org.eclipse.xtext.ui.tests.core.resource.Storage2UriMapperJavaImplTest.testGetStorages_Performance(Storage2UriMapperJavaImplTest.java:170)

@LorenzoBettini
Copy link
Contributor Author

Concerning UniqueClassNameValidatorUITest I add the waitForJdtIndex, let's see how it goes.

Concerning Storage2UriMapperJdtImplTest and Storage2UriMapperJavaImplTest there's no explicit waiting for build, and I don't know if and how the building is meant to be used in those tests. So, I'd need help about where to insert a waitForBuild if any.

@LorenzoBettini
Copy link
Contributor Author

The assertion failure for the storage 2 URI test was this one:

{platform:/resource/.org.eclipse.jdt.core.external.folders/externalFolder/a.indexed=NonJavaResource[a.indexed]} expected:<2> but was:<1>
java.lang.AssertionError: {platform:/resource/.org.eclipse.jdt.core.external.folders/externalFolder/a.indexed=NonJavaResource[a.indexed]} expected:<2> but was:<1>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.eclipse.xtext.ui.tests.core.resource.Storage2UriMapperJdtImplTest.testResourceInExternalFolder(Storage2UriMapperJdtImplTest.java:257)

let's see whether build synchronization fixes that one as well

@LorenzoBettini LorenzoBettini added this to the Release_2.38 milestone Jan 28, 2025
@LorenzoBettini LorenzoBettini merged commit c23f38b into eclipse-xtext:main Jan 28, 2025
9 checks passed
@LorenzoBettini LorenzoBettini deleted the lb_3319_flaky_tests branch January 28, 2025 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky UI tests Waiting for the JDT index
3 participants