-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
coverage: Allow specifying coverage flags via a yaml
file
#1954
base: main
Are you sure you want to change the base?
coverage: Allow specifying coverage flags via a yaml
file
#1954
Conversation
a9044f0
to
eebc68f
Compare
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR HealthBreaking changes ✔️
Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. Coverage ✔️
This check for test coverage is informational (issues shown here will not fail the PR). 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.
License Headers
|
Files |
---|
pkgs/coverage/lib/src/coverage_options.dart |
pkgs/coverage/test/collect_coverage_config_test.dart |
pkgs/coverage/test/config_file_locator_test.dart |
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/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/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
.
4580e6b
to
c3cbd55
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's some test failures. See if you can reproduce them locally.
https://github.com/dart-lang/tools/actions/runs/12686464325/job/35499098457?pr=1954
Hi @liamappelbe I've implemented the mentioned changes, and all test cases passed successfully locally. I look forward to your review. Thank you! |
…d search for options file
Pull Request Test Coverage Report for Build 12868001598Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there. Just a few more comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks good. Reviewing the tests now.
There's one more interesting bit of logic that deserves to be better tested: _findOptionsFilePath
. Make _findOptionsFilePath
public for testing, then write a unit test (in a separate test file) that tests all its edge cases. You'll also need to make some more directory structures and yaml files (maybe in a new directory under the test/
directory to keep things simple):
- Typical case: walk up a few directories to find the pubspec.yaml, and find the coverage_options.yaml file next to it
- No config case: walk up a few directories to find the pubspec.yaml, but there's no coverage_options.yaml file
- Edge case: when there's no pubspec.yaml file at all (this is tricky to test because any walk in the test directory is going to find package:coverage's pubspec, so maybe start in
Directory.systemTemp
)
The other issue with testing this function is that it starts in Directory.current
, which is not safe to alter in a test. So you'll need to change the currentDir
variable into an argument, and pass Directory.current
when invoking it from _getOptionsFile
. Then you can pass whatever you need to in the unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost done. Looks like the analyzer bot wants some fields in the test pubspecs. I think at a minimum you'll need the name
and environment
fields. Make sure to dart analyze
them before you upload, so you don't have to wait for the GitHub bots to verify you did it right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I'm ready to approve. But it looks like some tests are failing on CI. I can see you're passing recursive: true
when you create the output file, so I'm not sure why this is failing. You might need to do some debugging. Does it repro locally?
Hey @liamappelbe, Here’s what we can do to fix it:
Let me know your thoughts. |
@Dhruv-Maradiya You shouldn't need to add the files, and it would undermine the test slightly. I definitely have tests that create files on github CI. Maybe it just doesn't like creating directories? Git doesn't track empty directories, so even if they exist in your local repo, they won't exist on the bot. Try adding some empty files to those directories and commit them so that git tracks those directories. See if that fixes the issue. |
PR Description
Overview
Concerns
parseArgs Handling:
The
parseArgs
function is defined as private, preventing it from being imported into test files. As a workaround, I created a minimal version for testing purposes. I am wondering if there is a more efficient way to handle this.Test Coverage:
I have added test cases to validate the new functionality. I'm unsure if I have organized the tests properly and would appreciate feedback on their coverage and structure.