Skip to content

Conversation

@vogella
Copy link
Owner

@vogella vogella commented Nov 17, 2025

No description provided.

HeikoKlare and others added 30 commits November 5, 2025 08:17
When moved between monitors of different zoom, Dialogs may not show
their full contents anymore. This is caused by their shell and the used
fonts being linearly scaled in size according to the zoom, even though
font size and required area for a font of a size are not linearly
related. I.e., a font of double the size required more than double of
the width and height.

Since there are many ways to customize Dialogs and their sizes/layouts
during and after initialization, it's difficult to solve that problem in
general. It is, however, most severe for Dialogs that use the default
calculated size and are not resizable, as those Dialogs do not have
sophisticated layouts that adapt to the limited space and may easily
lead to cut offs the user cannot work around by resizing.
Thus, this change adds according zoom change and resize listeners to
identify if a Window uses the default computed size and, in that case,
recomputes it upon zoom change.
The test was failing intermittently on Windows with "expected: <1> but
was: <2>" because it was searching the entire project scope where files
from other concurrent tests could interfere.

Changes:
- Use unique timestamped folder name to avoid conflicts
- Narrow search scope to only the test's specific folder
- Add comments explaining the fix

This ensures test isolation when running in parallel.

Fixes eclipse-platform#3490

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
and is no longer referenced or used. Hence Removing it.
…op processing

This commit fixes intermittent test failures on macOS reported in GitHub issue eclipse-platform#294.

The root cause was a race condition between the test's explicit dialog.refresh()
call and asynchronous background jobs (FilterHistoryJob → FilterJob → RefreshCacheJob
→ RefreshJob) that populate the dialog content. Tests were checking selection state
before background jobs completed.

Changes:
- Added waitForDialogRefresh() helper method that processes UI events with delays
- Updated 10 tests to call waitForDialogRefresh() after dialog.refresh()
- 3 tests intentionally skip the wait to test edge cases with invalid selections

All 13 tests now pass consistently (verified with multiple runs).

Fixes: eclipse-platform#294

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
This commit removes unused JUnit TestName rule imports and field
declarations from three test classes:
- PartRenderingEngineTests.java
- TestUnitRegistrationWindows.java
- TextEditorPluginTest.java

The TestName fields were declared but never used in the test methods.
This cleanup also removes debug println statements that were left in
TestUnitRegistrationWindows.java.

Fixes: eclipse-platform#3451
ComparePreferencePage.colorAndFontLink were originally added for
emphasis in UI. Since the text already appears as a blue clickable link,
the additional quotes seems unnecessary. Removing the quotes makes the
message cleaner and consistent. This also aligns with other similar
preference links across the UI where such quotes are not used.
The previous fix in commit 9c171a0 added UITestUtil.processEvents()
to wait for decorator enablement, but this only processes the event
queue and doesn't wait for asynchronous jobs to complete.

The DecoratorManager.setEnabled() method triggers updateForEnablementChange()
which uses fireListenersInUIThread() to schedule WorkbenchJob instances.
These jobs may not complete before tests check decoration results, causing
intermittent failures.

This fix introduces waitForDecoratorJobs() which uses Job.getJobManager().join()
to wait for all FAMILY_DECORATE jobs to complete before proceeding with tests.

Fixes: eclipse-platform#868

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
The test was failing randomly due to a race condition with the
ProgressViewerComparator's stable sorting mechanism (lastIndexes).

Root cause:
- The comparator maintains a HashMap of previous positions to provide
  visual stability (preventing jobs from jumping around in the UI)
- During test execution, throttled updates would occur while jobs were
  being scheduled, causing some jobs to be added to lastIndexes before
  others
- This resulted in an inconsistent baseline where jobs were sorted by
  schedule order rather than priority order

Fix:
- Reopen the progress view after all jobs are scheduled and running to
  reset the comparator's lastIndexes state
- This ensures all jobs are sorted by priority from a clean state,
  eliminating the race condition
- Removed the unreliable timing-based approach that tried to manipulate
  throttled updates

The test now passes reliably by working with the stable sorting
behavior rather than against it.

Fixes eclipse-platform#195

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
The testKeepOneProperty test was failing randomly due to timing issues
when multiple jobs with KEEPONE_PROPERTY finish simultaneously.

Changes:
- Replace fixed 500ms wait with condition-based waiting that ensures
  all jobs have actually started (job.inProgress is true)
- Add explicit processEvents() calls after jobs complete and after
  marker job appears to ensure all UI event processing is complete
- Increase timeout from 500ms to 3000ms for job startup check

This eliminates the race condition by:
1. Ensuring all jobs are running before signaling them to finish
2. Ensuring job completion events are processed before checking state
3. Ensuring KEEPONE cleanup logic completes before final assertion

Tested with 10 consecutive runs - all passed.

Fixes eclipse-platform#370

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Bumps [actions/checkout](https://github.com/actions/checkout) from 4 to 5.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v4...v5)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-version: '5'
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [actions/setup-java](https://github.com/actions/setup-java) from 4 to 5.
- [Release notes](https://github.com/actions/setup-java/releases)
- [Commits](actions/setup-java@v4...v5)

---
updated-dependencies:
- dependency-name: actions/setup-java
  dependency-version: '5'
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Getting an editor of a MultpageEditorPart during disposal of the editor
led to dispose exceptions as getEditor is accessing an Item's data
without checking for disposal. This happened for example when during
shutdown the Web-kit widget is sending SWT events. (see
eclipse-4diac/4diac-ide#1808 for details). By
checking for disposal of the item. This issue is prevent.
The testEnabledWhenHover test was failing randomly when trying to retrieve
the hover shell for the second part of the test.

Root cause:
- The test has two parts: first with EnabledPropertyTester.setEnabled(true),
  then with setEnabled(false)
- After the first hover is shown and checked, cleanFileAndEditor() is called
- However, the hover shell from the first part may not be fully disposed
  when the second editor is opened and the second hover is triggered
- This causes getHoverShell() to timeout waiting for the new hover shell

Fix:
- Capture a reference to the first hover shell before calling cleanFileAndEditor()
- After cleanFileAndEditor(), explicitly wait for the first shell to be disposed
  using DisplayHelper.waitForCondition() with a 3000ms timeout
- This ensures the hover state is fully reset before the second part begins

This approach follows the pattern used in other recent flaky test fixes in
this repository (e.g., ProgressContantsTest, ProgressViewTests) which use
condition-based waiting to ensure proper cleanup between test phases.

Fixes eclipse-platform#926

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
- Auto-show first result in preview.
- Clear preview when search is empty removing stale entries.
The test was failing intermittently with "expected:<1> but was:<0>" because
it was asserting the KEEPONE property result before the async cleanup logic
had completed. The previous fix added an error job as a synchronization
marker, but this wasn't sufficient as the cleanup can still be pending after
the error job appears.

This fix adds explicit waits for the actual condition being tested: that
exactly 1 item from the DummyFamilyJob family remains in the progress view.
By waiting for countBelongingProgressItems() to return 1, we ensure the
KEEPONE cleanup has fully stabilized before asserting.

Fixes: https://github.com/eclipse-platform/eclipse.platform.ui/runs/55056915330

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
The testDynamicRegistry test was failing randomly with
"Activity Listener not called" or "Category Listener not called" errors.

Root cause:
- The test uses a plain boolean array to track listener invocations
- Extension registry listeners may be invoked on different threads
- Without proper synchronization (volatile/atomic), changes to the array
  made by listener threads may not be visible to the test thread
- This causes DisplayHelper.waitForCondition() to timeout even when
  listeners were actually called

Fix:
- Replace boolean[] with AtomicBoolean instances for thread-safe visibility
- Use activityChanged.set(true) and categoryChanged.set(true) in listeners
- Use activityChanged.get() and categoryChanged.get() in assertions
- AtomicBoolean provides proper memory barriers ensuring cross-thread visibility

This approach ensures proper synchronization between the listener callback
threads and the test thread, eliminating the race condition that caused
random test failures.

Fixes eclipse-platform#2458

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
This commit migrates the entire org.eclipse.ui.tests.navigator test
suite
from JUnit 4 to JUnit 5, modernizing the test infrastructure and
improving
test reliability with better resource management.

JUnit 5 Migration Changes:
- Updated all test annotations (@before@beforeeach, @after →
@AfterEach,
  @ignore@disabled)
- Migrated static imports from org.junit.Assert to
  org.junit.jupiter.api.Assertions
- Replaced CloseTestWindowsRule (JUnit 4) with inline window management:
    - Added @AfterEach tearDown() method
    - Inlined window listener and shell tracking logic
    - This avoids needing to add JUnit 5 dependencies to the test
harness bundle
- Fixed all assertion parameter orders from JUnit 4 style
(message-first) to
  JUnit 5 style (message-last):
  * assertEquals(message, expected, actual) → assertEquals(expected,
actual, message)
  * assertTrue(message, condition) → assertTrue(condition, message)
  * assertFalse(message, condition) → assertFalse(condition, message)

Test Infrastructure Improvements:

  manages UI test windows and shell cleanup, replacing the previous
JUnit 4
  rule-based approach
- Updated GoBackForwardsTest to use @RegisterExtension with the new
extension
- Ensures better test isolation by cleaning up leaked shells between
tests

Benefits:
- Uses modern JUnit 5 features and best practices
- Improved resource cleanup prevents test interference
- Consistent with Eclipse platform's migration to JUnit 5
- Better test isolation and reliability

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
This commit removes commented out code (dead code) from multiple files
across the codebase. The commented code was no longer serving any
documentation purpose and only added clutter.

Changes include:

Bundles:
- MarkersChangeListener.java: Removed 150+ lines of commented method
  implementations (needsUpdate, affectsCurrentState, etc.) and related
  helper methods. This code was originally commented out on 2009-11-30
  in commit b417bee (Bug 292525) with a note "left commented out for
  further investigation". After approximately 16 years, it is unlikely
  that further investigation will happen. Also removed the now-empty
  handleNoMarkerChange() method and its call site.
- MarkerContentGenerator.java: Removed commented if statement
- SelectionProcessor.java: Removed 27-line commented while loop block
  in multi-text selection paste handling
- CSSDocumentHandlerImpl.java: Removed multiple commented code blocks
  in startMedia, endMedia, startFontFace, endFontFace methods
- HeapStatus.java: Removed commented Kyrsoft view availability check

Tests:
- UIDialogs.java: Removed commented test implementations in
  testEditActionSetsDialog and testLoadNotExistingPerspective

All removed code was identified as dead code that doesn't serve
documentation purposes. Version control history preserves this code
if needed for future reference.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
…dition

The test was failing intermittently (1 out of 3 runs) due to a race
condition in content type registration. The test dynamically registers
a binary content type but files were created and searched before the
content type was fully available, causing the binary file to be
incorrectly included in search results.

This fix adds:
1. Explicit wait loop to verify content type is registered before
   proceeding (up to 500ms with 10ms intervals)
2. Force content type detection on files after creation by calling
   getContentDescription() to ensure binary signature describer is
   properly applied

Verified with 5 consecutive test runs - all passed.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
…ndition

The test was failing intermittently with "CleanupAddon should ensure that
partStack is not rendered anymore, as all childs have been removed" because
it was checking the cleanup result before the async cleanup logic had a
chance to execute.

Root cause:
- CleanupAddon performs cleanup asynchronously via Display.asyncExec()
  (CleanupAddon.java:352)
- When hidePart() is called, it triggers events that schedule async cleanup
  tasks on the event queue
- The test was calling waitForCondition() immediately after hiding parts,
  before the async cleanup tasks were even posted to the event queue
- This created a race where the wait could timeout before cleanup started

Fix:
- Add contextRule.spinEventLoop() after hiding partB and partC to ensure
  async cleanup tasks are processed before waiting for the condition
- Wrap waitForCondition() in assertTrue() to fail immediately with a clear
  message if cleanup doesn't complete within timeout
- Remove redundant assertFalse() statements

This matches the pattern used in the adjacent testBug332463 test which calls
spinEventLoop() after each hidePart() call.

Verified with 5 consecutive test runs - all passed (consistently completing
in ~0.028-0.036s).

Fixes eclipse-platform#751

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
The existing fix in eclipse/master (commit 87352d6) added a
waitForDialogRefresh() method but used a fixed-time wait (3 × 50ms = 150ms)
which was still insufficient on slower systems like macOS CI, causing
intermittent failures.

Root cause:
- FilteredResourcesSelectionDialog performs async operations (FilterHistoryJob
  → FilterJob → RefreshCacheJob → RefreshJob) after refresh()
- These jobs populate the table and apply initial selections asynchronously
- The original fix waited only 150ms, which is not enough on slow machines
- This caused the test to check selection before it was applied, resulting in:
  "expected:<[L/.../foo.txt]> but was:<[]>"

Improved fix:
- Changed waitForDialogRefresh() to use DisplayHelper.waitForCondition()
- Wait up to 2 seconds for table to be populated (condition-based, returns
  immediately when items appear)
- Then wait additional 250ms (5 × 50ms) for selection to be applied
- Total: condition-based wait + 250ms vs previous fixed 150ms
- Faster on fast systems (condition returns immediately when table populates)
- More reliable on slow systems (up to 2 seconds for table population)

This approach:
- Uses condition-based waiting for table population (more efficient)
- Provides adequate time for selection to be applied (250ms vs 150ms)
- Works reliably on both fast and slow systems
- Test time: ~9 seconds vs ~16 seconds with longer fixed waits

Verified with multiple consecutive test runs - all passed consistently.

Builds on the fix from commit 87352d6 which already added
waitForDialogRefresh() to the appropriate tests.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
The command labels in a perspective's onboarding area are placed in a
right-aligned layout but the labels themselves are not right-aligned,
such that they are not perfectly right aligned as they can be up to 1
point off (which can sum up to multiple pixels with monitor zoom
values).

This change adapts the labels to be right aligned such that they
properly fit.
vogella and others added 8 commits November 13, 2025 18:13
Introduce a Phaser to ensure async/sync operations are scheduled
before postStartup marks the workbench as started. This prevents
timing issues where test variables (asyncWithDisplayAccess,
syncWithDisplayAccess, etc.) might not be set correctly due to
the workbench being marked as started before async runnables execute.

Changes:
- Add Phaser to coordinate async/sync thread operations
- Register/deregister threads when scheduling display operations
- Wait for all operations to be scheduled in postStartup (with 5s timeout)
- Process event loop to ensure async runnables execute before marking started
- Simplify Boolean assignments (remove unnecessary ternary operators)

This ensures deterministic behavior in test harness initialization
and prevents flaky test failures due to race conditions.
The previous Phaser-based approach was timing out because:
1. Phaser only waited for operations to be scheduled, not executed
2. The inline event processing had issues pumping events reliably
3. Display.getCurrent() could be null in some contexts

This change uses CountDownLatch to wait for the actual execution
(not just scheduling) of async/sync operations with DisplayAccess.
The latch counts down inside the runnables after they execute,
ensuring they complete before postStartup() marks started=true.

Key improvements:
- Simpler synchronization model: wait for 2 operations to complete
- Count down after runnable execution (not after scheduling)
- Process remaining events after latch completes
- More reliable than Phaser for this use case

Fixes the following test failures that occurred with Phaser:
- TimeoutException waiting for operations to complete
- Async run during startup (was TRUE, should be FALSE)
- Sync from un-qualified thread ran during startup (was TRUE, should be FALSE)
- Async from un-qualified thread ran during startup (was TRUE, should be FALSE)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
…vent processing

The previous fix added a CountDownLatch to wait for operations with DisplayAccess,
but also included a display.readAndDispatch() loop that processed ALL pending events
in postStartup(). This caused deferred operations (without DisplayAccess) to execute
before startup completed, violating the contract that such operations should be
deferred until after startup.

This change removes the display.readAndDispatch() loop while keeping the latch-based
synchronization for operations with DisplayAccess. The test expects:
- Operations WITH DisplayAccess → run during startup
- Operations WITHOUT DisplayAccess → deferred until after startup
- Direct asyncExec from UI thread → deferred until after startup

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
The test was failing intermittently because QuickSearchQuickAccessComputer
performs asynchronous background searches with a 200ms timeout. On slower
CI systems (particularly macOS), this timeout was insufficient, causing
the test to fail when it checked for results before the search completed.

This fix:
- Saves the Display reference before closing the dialog to avoid NPE
- Wraps the QuickAccessComputer result check in DisplayHelper.waitForCondition
  with a 5-second timeout, retrying if no results are found initially
- Allows the background search job sufficient time to find matches

The test now passes consistently across multiple runs.

Fixes eclipse-platform#3042

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
concatenation

Apply low-risk performance optimizations across multiple bundles:

Loop optimizations - cache method results:
- StyledString: Cache otherRuns.size() before loop
- ContributionManager: Cache contributions.size() in indexOf() and
duplicate removal
- ContentProposalAdapter: Cache autoActivateString.length() in loop
- DialogSettings: Cache s.length() before character iteration
- MenuManagerShowProcessor: Cache menuModel.getChildren() to avoid
repeated calls
- WorkbenchPage: Cache getChildren() results to avoid expensive repeated
calls

String concatenation optimizations:
- WizardsRegistryReader: Use StringBuilder for path construction in loop
- NewContentTypeDialog: Use StringBuilder for name generation in while
loop

Repeated expensive calls:
- BindingManager: Cache format().length() results in comparison
- MenuManagerShowProcessor: Cache getChildren() in loop condition and
body

All changes are non-API breaking and maintain identical behavior while
reducing unnecessary object allocations and method calls.
…ndition

The test was failing intermittently on Windows CI with "CleanupAddon should
ensure that partStack is not rendered anymore, as all childs have been removed"
because it was checking the cleanup result before the async cleanup logic had
a chance to execute.

Root cause:
- CleanupAddon performs cleanup asynchronously via Display.asyncExec()
  (CleanupAddon.java:352)
- The test called contextRule.spinEventLoop() immediately after hiding parts
  (line 2469), which processes only currently queued events
- This created a race condition: spinEventLoop() might process events before
  CleanupAddon's asyncExec() callback was even added to the event queue
- The premature event processing caused timing issues that made the subsequent
  DisplayHelper.waitForCondition() unreliable

Fix:
- Remove the contextRule.spinEventLoop() call on line 2469
- Rely solely on DisplayHelper.waitForCondition(), which properly handles event
  processing via Display.sleep() with a 10ms retry interval
- Simplify the condition from "isToBeRendered() == false" to "!isToBeRendered()"

This follows the pattern from recent race condition fixes:
- Commit 55574d8: "removing premature event processing" for RCPTestWorkbenchAdvisor
- Commit 7c0684c: Using DisplayHelper.waitForCondition for async operations

Verified with 5 consecutive test runs - all passed consistently (0.028-0.036s).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
This branch demonstrates how to reliably reproduce the race condition
that was fixed in commit 33748d2.

What was changed:
Added a Thread.sleep(500) after tableViewer.refresh() in
FilteredItemsSelectionDialog.refresh() method (line 890-894).

Why this reproduces the race condition:
- On slow systems, tableViewer.refresh() takes time to complete its
  async table updates
- The injected 500ms delay simulates this slow behavior
- When setSelection() is called after the delay (line 903), the table
  items are not yet ready
- The selection fails silently, resulting in empty selection

How to use this branch:
1. Checkout this branch: git checkout demo-race-condition
2. Compile: mvn clean compile -Pbuild-individual-bundles
3. Run tests: mvn verify -Dtest=ResourceInitialSelectionTest
4. Observe failures:
   - "One file should be selected by default expected:<[...foo.txt]> but was:<[]>"
   - "Two files should be selected by default"

Expected test failures:
- testSingleSelectionAndOneInitialSelectionWithInitialPattern
- testMultiSelectionAndSomeInitialNonExistingSelectionWithInitialPattern
- testMultiSelectionAndTwoInitialSelectionsWithInitialPattern
- And possibly others

To see the fix:
git checkout resourcetest (or the branch with the fix)

The fix uses Display.asyncExec() to defer selection application until
after the table refresh completes, eliminating the race condition.

⚠️ DO NOT MERGE THIS BRANCH - FOR DEMONSTRATION ONLY

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@gemini-code-assist
Copy link

Summary of Changes

Hello @vogella, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request encompasses a series of changes aimed at enhancing the Eclipse UI platform's stability, maintainability, and developer experience. It includes significant code cleanup by removing obsolete comments and code, updating deprecation markers for better API guidance, and introducing UI responsiveness improvements. A major focus is on addressing and preventing race conditions, particularly within UI dialogs and test suites, to ensure more reliable behavior and consistent test results. Additionally, it updates build configurations and modernizes test infrastructure by migrating to JUnit 5.

Highlights

  • Documentation Updates: Updated the AGENTS.md file to reflect the use of Java 21 and Maven 3.9.11, and removed an outdated critical limitation note regarding parent POM resolution.
  • Code Cleanup and Refactoring: Removed numerous blocks of commented-out code across several files, including CSSDocumentHandlerImpl.java, SelectionProcessor.java, TextSearchPage.java, MarkerContentGenerator.java, MarkersChangeListener.java, HeapStatus.java, and UIDialogs.java, improving code readability and maintainability.
  • Deprecation Annotation Enhancements: Updated @Deprecated annotations in many JFace Text and UI classes (e.g., HTMLPrinter.java, TextViewer.java, DefaultInformationControl.java, AnnotationBarHoverManager.java, ContentAssistHandler.java, SaveAction.java) to include forRemoval=true, since="2025-12", providing clearer guidance on future removal.
  • UI Performance and Stability Improvements: Optimized processDynamicElements in MenuManagerShowProcessor.java and resetPerspective in WorkbenchPage.java to avoid redundant getChildren() calls, potentially improving UI responsiveness. A new hookZoomChangeListenerForDefaultSizedDialogs method was added to Dialog.java to ensure dialogs correctly adapt their size on zoom changes, preventing layout issues.
  • Quick Search Dialog Enhancements: Added a null check for targetLineFirstMatch and implemented auto-selection of the first search result in QuickSearchDialog.java when the list is populated. The preview section is now cleared when the search pattern is empty.
  • Test Reliability and Race Condition Mitigation: Introduced waitForDecoratorJobs in DecoratorAdaptableTests.java and waitForDialogRefresh in ResourceInitialSelectionTest.java to ensure UI updates and background jobs complete before assertions, preventing intermittent test failures due to race conditions. Similar synchronization logic was added to QuickAccessComputerTest.java and PartRenderingEngineTests.java. A Thread.sleep was explicitly added to FilteredItemsSelectionDialog.java to demonstrate a race condition.
  • JUnit 5 Migration and Test Modernization: Updated MANIFEST.MF for org.eclipse.ui.monitoring.tests to JUnit Jupiter and migrated numerous test classes (e.g., DefaultLoggerTests.java, EventLoopMonitorThreadManualTests.java, GoBackForwardsTest.java) from JUnit 4 to JUnit 5, including changes to imports, annotations (@Before to @BeforeEach, Assert to Assertions), and assertion syntax.
  • Build and Tooling Updates: Updated the Yourkit P2 repository URL in platformUiTools.p2f.
Ignored Files
  • Ignored by pattern: .github/workflows/** (1)
    • .github/workflows/copilot-setup-steps.yml
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request includes a diverse set of improvements, such as removing commented-out code, updating deprecated APIs, and migrating numerous tests from JUnit 4 to JUnit 5. Many tests have been made more robust by addressing race conditions and better handling asynchronous behavior. The production code also benefits from refactorings that enhance performance and readability.

A key point of concern is the intentional introduction of a Thread.sleep() in FilteredItemsSelectionDialog.java to demonstrate a race condition. While this is useful for debugging, it is critical that this code is removed before merging.

Comment on lines +876 to +894
// ⚠️ DEMONSTRATION: Inject delay to make race condition reproducible
// This simulates what happens on slow systems where tableViewer.refresh()
// takes longer to complete. On fast systems, the race condition is intermittent
// because refresh() usually completes quickly enough. This delay makes it
// fail consistently by preventing the table from being ready when setSelection()
// is called below.
//
// To see the race condition:
// 1. Compile this code
// 2. Run ResourceInitialSelectionTest
// 3. Tests will fail with: expected:<[...foo.txt]> but was:<[]>
//
// This demonstrates the root cause that was fixed by using Display.asyncExec()
// in commit 33748d2f3a.
try {
Thread.sleep(500); // 500ms delay - makes race condition 100% reproducible
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}

Choose a reason for hiding this comment

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

critical

This code block, which includes a Thread.sleep(500), was intentionally added to demonstrate a race condition, as explained in the accompanying comments. While this is a useful technique for debugging and testing, it must be removed before this pull request is merged into the main branch. Including a hardcoded sleep in production code introduces a significant performance bottleneck and is not acceptable.

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.

10 participants