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

Don't wait for jobs in SmartImportTests #1810

Conversation

fedejeanne
Copy link
Contributor

@fedejeanne fedejeanne commented Apr 10, 2024

Remove calls to waitForJobs(...) in SmartImportTests and only join the (now correct) SmartImportJob instead.

Rename SmartImportWizard::getImportJob to createOrGetConfiguredImportJob and change its javadoc (the method was doing a lot more than simply retrieving a Job, which is what broke the tests in the first place).
Add a new method SmartImportWizard::getCurrentImportJob that merely returns the current job and join that job in the tests. Before this PR, a new job was being created and joined, which didn't make sense since that job was never started in the first place.

Fixes: #1427
Fixes: #1807

Copy link
Contributor

github-actions bot commented Apr 10, 2024

Test Results

   921 files  ±0     921 suites  ±0   1h 10m 20s ⏱️ + 23m 41s
 7 514 tests ±0   7 362 ✅ +1  151 💤 ±0  1 ❌  - 1 
23 694 runs  ±0  23 186 ✅ +1  507 💤 ±0  1 ❌  - 1 

For more details on these failures, see this check.

Results for commit ffc46be. ± Comparison against base commit 016247d.

♻️ This comment has been updated with latest results.

@HeikoKlare
Copy link
Contributor

Can you explain how removing the waiting for job completion is supposed to fix the referenced tests? In particular, the failure in #1807 seems to be because some project has not been imported, which is nothing I would expect to succeed after removing wait operations.

However, at least the first waitForBuilds() call in the tests seems to be obsolete, because the job is scheduled when finishing the wizard and thus the join() will block until the job has finished anyway.

@fedejeanne
Copy link
Contributor Author

The wrong job was being joined, I changed the description of this PR to explain that.

@fedejeanne
Copy link
Contributor Author

I'll squash the commits before merging, I'll let the tests run for now.

@fedejeanne fedejeanne force-pushed the bugs/random_failings_in_SmartImportTest branch from a77cce6 to e033414 Compare April 12, 2024 12:12
- Rename "getImportJob" to "createOrGetConfiguredImportJob" to better
express its behavior and also change its JavaDoc.
- Add a new method "getCurrentImportJob" that merely retrieves the
current job (without creating/configuring anything).
- Extract private method in SmartImportTests
- Remove calls to waitForJobs(...) in SmartImportTests and join the
SmartImportJob instead.

Fixes: eclipse-platform#1427
Fixes: eclipse-platform#1807
@fedejeanne fedejeanne force-pushed the bugs/random_failings_in_SmartImportTest branch from e033414 to ffc46be Compare April 12, 2024 12:15
@fedejeanne fedejeanne merged commit 85f59c5 into eclipse-platform:master Apr 12, 2024
14 of 16 checks passed
@fedejeanne fedejeanne deleted the bugs/random_failings_in_SmartImportTest branch April 12, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants