Repository navigation
Ignore child popups in CompletionTest.findNewShell #3880 - #4491
Open
ravnskjaer wants to merge 2 commits into
Open
ravnskjaer wants to merge 2 commits into
ravnskjaer wants to merge 2 commits into
Conversation
The additional proposal info popup, a child of the proposal popup, could be counted as a second new shell when the UI thread was delayed, failing testMoveCaretBackUsesAllProcessors_bug522255. Count only top-level shells. Also re-enable testMoveCaretBackUsesAllProcessors_bug522255 on macOS, where it was disabled for eclipse-platform#906. See eclipse-platform#3880
| List<Shell> topLevelNewShells = newShells.stream() | ||
| .filter(shell -> !newShells.contains(shell.getParent())) | ||
| .toList(); | ||
| assertTrue(topLevelNewShells.size() <= 1, "More than one new top-level shell was found"); |
Contributor
There was a problem hiding this comment.
Please add the shell titles to the fail message.
Contributor
Author
There was a problem hiding this comment.
Good idea, done in a separate commit, so the change is easy to review; I'll squash it once you're happy with it. Since content assist popups have an empty title, I also added the type of each shell's child controls. Checked by temporarily disabling the child-popup filter, a failure now looks like:
More than one new top-level shell was found: ['' [Table], '' [Composite]]
Contributor
Author
There was a problem hiding this comment.
@trancexpress are you ok with the changes, can I resolve the comment?
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
testMoveCaretBackUsesAllProcessors_bug522255sporadically fails with "More than one new shell was found" (on the macOS CI job of my fork; also reported on Windows in #3880).Cause: When the UI thread is slow, the test's 200 ms wait can last longer than the post-selection delay (
OpenStrategy.getPostSelectionDelay(), 500 ms). The additional proposal info popup (a child of the proposal popup) then opens next to the selected "Computing proposals" entry, andfindNewShellcounts it as a second shell. With temporary debug output listing the new shells (not part of this PR), the failure looked like this:Reproduced locally by extending the
sleep(…, 200)beforefindNewShellintestMoveCaretBackUsesAllProcessors_bug522255to 800 ms.In the failures I saw, the failing iteration took noticeably longer than the passing ones (about 2 s instead of about 1 s), even though a failing iteration stops early.
Fix:
findNewShellignores new shells whose parent is another new shell. The test is also re-enabled on macOS (disabled since 2023 for #906): in ~300 iterations on the macOS CI job of my fork, this race was the only failure. The remaining four tests follow in a separate PR.Verification: with the sleep at 800 ms, 50/50 iterations failed without the fix and 50/50 passed with it.
See #3880