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

Make completion proposal calculation test time-independent #906 #967

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HeikoKlare
Copy link
Contributor

@HeikoKlare HeikoKlare commented Jul 25, 2023

The test cases for completion proposals in CompletionTest depend on specific timing behavior of a test proposal processor. On one hand, this is prone to errors if proper timing is not given on the test environment. On the other hand, the tests run unnecessarily long because of waiting long enough.

This change replaces the fixed-time waiting of the LongRunningBarContentAssistProcessor with an explicit trigger that finalizes the content calculation as soon as some barrier is reached within the tests. In addition, the long-running processor is only enabled on demand and will be finalized whenever a test case was executed to avoid that the processor thread is leaking to the next test case.

Contributes to #906

@github-actions
Copy link
Contributor

github-actions bot commented Jul 25, 2023

Test Results

     852 files  ±0       852 suites  ±0   1h 5m 26s ⏱️ - 5m 36s
  7 207 tests ±0    7 054 ✔️  - 2  149 💤 ±0  4 +2 
22 767 runs  ±0  22 275 ✔️  - 3  487 💤 ±0  5 +3 

For more details on these failures, see this check.

Results for commit 0b34f69. ± Comparison against base commit 05c67f7.

♻️ This comment has been updated with latest results.

Comment on lines 46 to 47
assertFalse("Concurrent requests for finishing computation of long running content assist processor", shouldComputationFinish);
shouldComputationFinish = true;
Copy link
Contributor

@fedejeanne fedejeanne Jul 26, 2023

Choose a reason for hiding this comment

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

This could allow 2 threads to call finishComputation and NOT fail if they both reach line 47 "at the same time".

You could either add synchronized to this method (which would NOT get rid of a race condition in the while-loop in line 35) or use an AtomicBoolean and call assertFalse(..., shouldComputationFinish.getAndSet(false)) instead.

}
shouldComputationFinish = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary? The field is private i.e. no one will read it afterwards

@HeikoKlare HeikoKlare force-pushed the issue-906-performance branch 2 times, most recently from 286908a to 99f9419 Compare July 27, 2023 14:25
@HeikoKlare HeikoKlare changed the title Make completion proposal calculation test time-independent #906 #907 Make completion proposal calculation test time-independent #906 Jul 27, 2023
…atform#906

The test cases for completion proposals in CompletionTest depend on
specific timing behavior of a test proposal processor. On one hand, this
is prone to errors if proper timing is not given on the test
environment. On the other hand, the tests run unnecessarily long because
of waiting long enough.

This change replaces the fixed-time waiting of the
LongRunningBarContentAssistProcessor with an explicit trigger that
finalizes the content calculation as soon as some barrier is reached
within the tests. In addition, the long-running processor is only
enabled on demand and will be finalized whenever a test case was
executed to avoid that the processor thread is leaking to the next test
case.

Contributes to
eclipse-platform#906
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.

None yet

2 participants