Skip to content

Conversation

davidmorgan
Copy link
Contributor

@davidmorgan davidmorgan commented Sep 29, 2025

For #2170, start adding test coverage.

I initially started adding test coverage around symlinks, but encountered two existing issues that made it awkward

  • there is a race on startup on MacOS that means there might be events from before the watch; the existing tests are flaky because of this issue
  • the file watchers have inconsistent behavior if the file watched does not exist

Cover both of these with tests before making any changes :)

Add expectNoEvents and use it in one existing test as it's more readable than triggering then waiting for a different event.

@davidmorgan davidmorgan requested a review from a team as a code owner September 29, 2025 11:40
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 adds valuable test coverage for a startup race condition and for watching non-existent files. The refactoring of shared tests and the addition of the expectNoEvents helper are good improvements. However, I've identified a couple of critical issues in the new tests. The test for the startup race condition is not functioning as intended due to misuse of global test helpers, and another test has an incorrect expect call. Addressing these issues will make this a strong contribution to the test suite's reliability.

Copy link

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
watcher None 1.1.3 1.1.3 1.1.3 ✔️

This check can be disabled by tagging the PR with skip-breaking-check.

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

Coverage ✔️
File Coverage

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbol Leaking sources

This check can be disabled by tagging the PR with skip-leaking-check.

License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/bazel_worker/benchmark/benchmark.dart
pkgs/bazel_worker/example/client.dart
pkgs/bazel_worker/example/worker.dart
pkgs/benchmark_harness/integration_test/perf_benchmark_test.dart
pkgs/boolean_selector/example/example.dart
pkgs/clock/lib/clock.dart
pkgs/clock/lib/src/clock.dart
pkgs/clock/lib/src/default.dart
pkgs/clock/lib/src/stopwatch.dart
pkgs/clock/lib/src/utils.dart
pkgs/clock/test/clock_test.dart
pkgs/clock/test/default_test.dart
pkgs/clock/test/stopwatch_test.dart
pkgs/clock/test/utils.dart
pkgs/coverage/lib/src/coverage_options.dart
pkgs/html/example/main.dart
pkgs/html/lib/dom.dart
pkgs/html/lib/dom_parsing.dart
pkgs/html/lib/html_escape.dart
pkgs/html/lib/parser.dart
pkgs/html/lib/src/constants.dart
pkgs/html/lib/src/encoding_parser.dart
pkgs/html/lib/src/html_input_stream.dart
pkgs/html/lib/src/list_proxy.dart
pkgs/html/lib/src/query_selector.dart
pkgs/html/lib/src/token.dart
pkgs/html/lib/src/tokenizer.dart
pkgs/html/lib/src/treebuilder.dart
pkgs/html/lib/src/utils.dart
pkgs/html/test/dom_test.dart
pkgs/html/test/parser_feature_test.dart
pkgs/html/test/parser_test.dart
pkgs/html/test/query_selector_test.dart
pkgs/html/test/selectors/level1_baseline_test.dart
pkgs/html/test/selectors/level1_lib.dart
pkgs/html/test/selectors/selectors.dart
pkgs/html/test/support.dart
pkgs/html/test/tokenizer_test.dart
pkgs/html/test/trie_test.dart
pkgs/html/tool/generate_trie.dart
pkgs/pubspec_parse/test/git_uri_test.dart
pkgs/stack_trace/example/example.dart
pkgs/watcher/test/custom_watcher_factory_test.dart
pkgs/yaml_edit/example/example.dart

This check can be disabled by tagging the PR with skip-license-check.

@davidmorgan davidmorgan requested review from chloestefantsova and removed request for a team September 29, 2025 12:59
@davidmorgan davidmorgan requested a review from a team as a code owner September 29, 2025 15:18
@davidmorgan
Copy link
Contributor Author

Whoops, some of the new tests were not actually wired up to run--because I copied them from the PR where I originally wrote them.

I will fix and resend for review :)

@davidmorgan davidmorgan removed request for chloestefantsova and a team September 29, 2025 15:24
@davidmorgan davidmorgan force-pushed the more-watcher-tests branch 4 times, most recently from 14e126d to d28c2f9 Compare September 30, 2025 06:56
@davidmorgan
Copy link
Contributor Author

Okay, this looks good now. The test failures are unrelated, pre-existing flakiness: they're on directory_watcher, this PR is just on file_watcher.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants