Skip to content

Fix flaky test_renaming_top_level_directory on Windows#1179

Open
Arman-Luthra wants to merge 1 commit into
gorakhargosh:masterfrom
Arman-Luthra:fix-flaky-renaming-top-level-directory
Open

Fix flaky test_renaming_top_level_directory on Windows#1179
Arman-Luthra wants to merge 1 commit into
gorakhargosh:masterfrom
Arman-Luthra:fix-flaky-renaming-top-level-directory

Conversation

@Arman-Luthra

Copy link
Copy Markdown

Addresses the Windows failure mode of #1128.

Root cause

On Windows, mkdir(p("a", "b")) updates the last-write time of directory a, but NTFS flushes that change - and thus delivers the corresponding FILE_ACTION_MODIFIED notification for a - lazily. Instrumenting DirectoryChangeReader directly shows the notification is not delivered when the subdirectory is created; on my machine it only shows up when the subsequent mv(p("a"), p("a2")) touches the directory, ~4 seconds later:

[  3.031s] mkdir a/b
  [  3.266s] action=1 (ADDED)        src='a\b'
[  7.031s] rename a -> a2
  [  7.313s] action=3 (MODIFIED)     src='a'    <- delayed notification from the mkdir
  [  7.313s] action=4 (RENAMED_OLD)  src='a'
  [  7.313s] action=5 (RENAMED_NEW)  src='a2'
  [  7.313s] action=3 (MODIFIED)     src='a2'

Two consequences for the test:

  1. The modified event for a arrives in the events-checker block for the rename, not the block for the mkdir - or, on other machines (apparently the GitHub runners), not at all.
  2. When it arrives after the rename, a no longer exists on disk anymore, so WindowsApiEmitter.queue_events() classifies it as a FileModifiedEvent instead of a DirModifiedEvent (src/watchdog/observers/read_directory_changes.py line 89 - the os.path.isdir(src_path) check runs at processing time, not at event time).

The events checker requires exactly the expected events, in order, so the stray event makes the test fail with AssertionError: wrong number of events.

Fix

Test-side only. The emitter cannot reliably classify a modified event for a path that no longer exists without the extended info from ReadDirectoryChangesExW, which is what #1156 is introducing for deletions - I did not want to conflict with that work.

  • _EventsChecker.add() accepts optional=True for events whose delivery is not deterministic on the platform; both the ordered and unordered checks now tolerate absent optional events. The ordered check is rewritten as a two-pointer walk over found/expected events (the previous zip()-based check also silently ignored missing trailing events whenever allow_extra_events() was used).
  • expected_class may now be a tuple of classes (passed straight through to isinstance()). This is needed because the class of the delayed event depends on whether the directory still exists when the emitter processes the notification (see point 2 above).
  • test_renaming_top_level_directory declares the delayed notification for a as optional in the two affected blocks.

Before/after

On this machine (Windows 11, NTFS, Python 3.12.10), tests/test_emitter.py::test_renaming_top_level_directory:

  • unmodified master: failed 9/9 runs;
  • with this change: passed 7/7 runs.

Full test suite apart from test_0_watchmedo: 102 passed, 12 skipped. test_0_watchmedo had one unrelated flaky failure (test_auto_restart_on_file_change_debounce, thread-count assertion) which also fails on master CI on Linux 3.13t; it passes on rerun and does not use the events checker. ruff check is clean on the touched files.

Limitations

#1128 also shows a Linux/Python 3.14 failure (DirMovedEvent not delivered within the 2 s timeout, pre-dating the events-checker refactor). I could not reproduce that on this machine and this PR does not address it - only the Windows failure mode, which is deterministic here.

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.

1 participant