Skip to content

Drain file watcher events during test setup#24030

Merged
zsol merged 1 commit intomainfrom
zsol/jj-tpwxroytnprq
Mar 18, 2026
Merged

Drain file watcher events during test setup#24030
zsol merged 1 commit intomainfrom
zsol/jj-tpwxroytnprq

Conversation

@zsol
Copy link
Member

@zsol zsol commented Mar 18, 2026

See astral-sh/ty#1540, I believe this PR should solve the remaining two issues in there.

@astral-sh-bot astral-sh-bot bot added the ty Multi-file analysis & type inference label Mar 18, 2026
@zsol zsol changed the title Attempt to fix flaky file watched test Attempt to fix flaky file watcher test Mar 18, 2026
@astral-sh-bot
Copy link

astral-sh-bot bot commented Mar 18, 2026

Typing conformance results

No changes detected ✅

Current numbers
The percentage of diagnostics emitted that were expected errors held steady at 85.29%. The percentage of expected errors that received a diagnostic held steady at 78.13%. The number of fully passing files held steady at 64/132.

@astral-sh-bot
Copy link

astral-sh-bot bot commented Mar 18, 2026

Memory usage report

Memory usage unchanged ✅

@astral-sh-bot
Copy link

astral-sh-bot bot commented Mar 18, 2026

ecosystem-analyzer results

No diagnostic changes detected ✅

Full report with detailed diff (timing results)

@AlexWaygood AlexWaygood added the testing Related to testing Ruff itself label Mar 18, 2026
@zsol zsol marked this pull request as ready for review March 18, 2026 13:11
// This ensures the FSEvents stream (on macOS) is fully started before the test
// proceeds, preventing flakes where events are lost during stream restart.
let sentinel_path = project_path.join(".watcher_ready");
std::fs::write(sentinel_path.as_std_path(), "ready")?;
Copy link
Member

Choose a reason for hiding this comment

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

Can you say more why this should fix the issue? I'm not very convinced that it does :)

let changes = case.stop_watch(event_for_file("baz.py"));

does wait for a file system event related to baz.py, which we did observe in https://github.com/astral-sh/ruff/actions/runs/20860850768/job/59939819903?pr=22481, but the test still failed.

If the concern really is that it sometimes takes too long for the file watcher to start, then the solution is to bump the default timeout here

self.try_stop_watch(matcher, Duration::from_secs(10))

Copy link
Member Author

@zsol zsol Mar 18, 2026

Choose a reason for hiding this comment

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

I think there are two things going for the approach in this PR:

  1. it ensures that the watcher is working well, and not e.g. backed up with events from before the current test
  2. flushes events unrelated to the current test a lot better than the previous test_case.try_take_watch_changes(|_| true, ...)

I suspect what's going on in https://github.com/astral-sh/ruff/actions/runs/20860850768/job/59939819903?pr=22481 is that there are multiple events emitted by the file watcher that aren't related to the symlink_inside_project test case, but still leak into them.

Edit: open to suggestions about how to better explain this in the comment :)

Copy link
Member

Choose a reason for hiding this comment

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

Thank you. I think we can copy exactly what you wrote here into the comment.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thanks. Let's give this a try

// This ensures the FSEvents stream (on macOS) is fully started before the test
// proceeds, preventing flakes where events are lost during stream restart.
let sentinel_path = project_path.join(".watcher_ready");
std::fs::write(sentinel_path.as_std_path(), "ready")?;
Copy link
Member

Choose a reason for hiding this comment

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

Thank you. I think we can copy exactly what you wrote here into the comment.

@carljm carljm removed their assignment Mar 18, 2026
@carljm carljm removed their request for review March 18, 2026 13:57
@zsol zsol force-pushed the zsol/jj-tpwxroytnprq branch from 4578a19 to e986d47 Compare March 18, 2026 14:03
@zsol zsol changed the title Attempt to fix flaky file watcher test Drain file watcher events during test setup Mar 18, 2026
@zsol zsol merged commit f99b284 into main Mar 18, 2026
49 checks passed
@zsol zsol deleted the zsol/jj-tpwxroytnprq branch March 18, 2026 14:14
carljm added a commit that referenced this pull request Mar 25, 2026
* main:
  [`pylint`] Improve phrasing (`PLC0208`) (#24033)
  test: migrate `show_settings` and `version` tests to use `CliTest` (#23702)
  Drain file watcher events during test setup (#24030)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Related to testing Ruff itself ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants