fix for flaky editor sync test #2385

[mehmet-karaman]

added a sleep before assert.. Tested on redhat 9 .. works as before.. tested on mac sonoma 14.6.1 .. works as before.. Needs observation

[github-actions]

Test Results

  6 460 files  +12    6 460 suites  +12   3h 6m 19s ⏱️ + 1m 24s
 43 242 tests +72   42 658 ✅ +72    584 💤 ±0   0 ❌ ±0 
170 275 runs  +80  167 898 ✅ +72  2 358 💤 ±0  19 ❌ +8 

Results for commit 08293be. ± Comparison against base commit a457f45.

♻️ This comment has been updated with latest results.

[mehmet-karaman]

@LorenzoBettini I've added a sleep of 250ms .. There was an unused and deprecated method, but for my case its not a build process, so i really needed sleeping thread. I am not sure if this helps..

[LorenzoBettini]

IMHO, adding sleeping in tests is very bad design ;)

I don't understand the deprecated method you mention.

Maybe the problem is that you change the file with Java API and you don't refresh the corresponding eclipse resource?

[mehmet-karaman]

Thats the sense of that syncronize :) It must recognize that the file was changed and change the content of the editor.

[mehmet-karaman]

Is it ok, to add polling and sleep (5-10ms) to minimize the sleeping? For example wait in total 200ms but poll all 5ms and check if the assert is true.. if after 200ms the value isn't true, fail the test? pollingAssertThat()?

[LorenzoBettini]

sleeping is not real synchronization ;)

If I understand the test and the implemented functionality correctly, it should be enough to trigger a refresh on the corresponding Eclipse resource, before doing the assert.

[LorenzoBettini]

@mehmet-karaman I'm closing the PR and reopen it so that it builds against the new main branch where there's a fix for tests of web projects.

[LorenzoBettini]

@mehmet-karaman If I understand correctly:

• previously, the test used to modify the file externally and immediately check that the editor was NOT yet synchronized; that relied on the quick assertFalse, but since by default Eclipse now automatically listens for changes in the file system, the test used to be flaky
• now, after modifying the file externally, you trigger a refresh and immediately assert that the editor IS synchronized. So, you don't check that it was not synchronized, but the previous assertFalse was not critical for the test, am I right?

If I got it correctly, then Job.getJobManager().join(ResourcesPlugin.FAMILY_AUTO_REFRESH, null); and syncUtil.waitForBuild(new NullProgressMonitor()); are useless before checking the editor's contents, am I right?

[mehmet-karaman]

The synchronize is only false for the first time, as it also triggers the synchronize. If it happens outside of my test somehow, it can cause flakyness (as it is only false for the first time executing this method) .. So there is no doubt that we can remove it, the critical part for this test is just the editors document content.

The "Job.getJobManager().join(ResourcesPlugin.FAMILY_AUTO_REFRESH, null); " could be necessary because it the refresh job should be done before checking the document content.
the waitForBuild is unnecessary, because after the refresh the build is done with the newest content. So we can skip this only.

[mehmet-karaman]

Ah maybe you meant "syncUtil.waitForReconciler(editor);" .. I've also removed that.. same reason as waitForBuild..

[LorenzoBettini]

To me the PR looks good. @szarnekow wdyt?

[mehmet-karaman]

Its still flaky.. I guess on macosx its a little bit slower with the refresh, so I am going to add another wait for refresh job.. I hope that fixes the test problem for macosx.

[LorenzoBettini]

@mehmet-karaman flaky on your machine or here on the CI? If it's on the CI, are you sure the flaky test is the one mentioned here?

[mehmet-karaman]

The last macos build failed on the same test: https://github.com/eclipse/xtext/actions/runs/11215846276

[LorenzoBettini]

Isn't refresh synchronous? I mean, when it returns the resource should be already refreshed, shouldn't it?

[mehmet-karaman]

I am out of home for this week and i don't have access to my private mac machine. On Linux it works locally and on remote build fine. But on mac remote build it had a problem, which seems to work with the latest change.

The next possible time to test this for me on mac is on Saturday evening.

[mehmet-karaman]

Could test it on mac. The refresh is synchronous (states also in the comments that it is long running). Removed the unnecessary refresh job wait after the refreshLocal-method call..

[LorenzoBettini]

Thanks @mehmet-karaman