Add .fromEnvironment flags to the global declarer#2561
Add .fromEnvironment flags to the global declarer#2561johnniwinther wants to merge 3 commits intomasterfrom
Conversation
The global declarer is used by package:test_reflective_loader which has no means of configuring the test output. It is for instance used to run all tests in package:analyzer, and the current output prints a line for each test (of which there is currently >32000) and uses colors in the output. This makes it very hard to output to find failing tests, since the failures are hidden within all the succeeding tests and piping the output to a file for easier search still leaves the color in the output. This change adds .fromEnvironment variables to the creation of the global declarer, such that it can be configured at the command line. This allow for enabling compact output which only displays the failing tests, and whether to use colors in the output.
PR HealthChangelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
| @@ -1,3 +1,6 @@ | |||
| ## 0.6.14 | |||
| * Support environment flags for configuring the global declarer. | |||
There was a problem hiding this comment.
Should be a bit more explicit here?
It'd also be nice to update this section of the readme – so this feature isn't lost.
https://github.com/dart-lang/test/tree/master/pkgs/test#selecting-a-test-reporter
I have a few packages where I'd use this, too!
|
There are many more issues than just this with running tests directly as opposed to through the test runner - test annotations won't be respected and test configuration files also won't be respected (and probably many more such as timeouts etc). Is it possible to just use the test runner here? |
|
I also think it is potentially confusing that these environment variables are only supported by the global declarer and don't more generally set the defaults of test reporters as a whole. |
natebosch
left a comment
natebosch
left a comment
There was a problem hiding this comment.
Let me know if you'd prefer I open a PR with an implementation matching my comments. I think it is reasonable to support at least one or two of the Dart compile environment variables.
| @@ -0,0 +1,32 @@ | |||
| // Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file | |||
There was a problem hiding this comment.
What is the purpose of this file? Is there a particular reason is it written so non-idiomatically for a Dart unit test?
Can we get by with a String in the file that needs this?
For historical reasons most of the test for test_core are still living under the test package so most of the utility code for running meta-tests is there. It might be easier to locate this test there for now as well.
| }); | ||
| }); | ||
|
|
||
| group('printPath', () { |
There was a problem hiding this comment.
Do we have a specific use case in mind for the printPath and printPlatform configuration? I cannot imagine a use case where the invoker of Dart binary wouldn't be a better place to deal with knowledge of the platform for instance.
I think it's preferable to only add configuration support for the specific values we need (I think that's control over the reporter and use colors IIRC?) and we can add further support later if we need to.
| List<Pattern> contains = const [], | ||
| List<String>? doesNotContain, | ||
| }) async { | ||
| if (debugPrint) { |
There was a problem hiding this comment.
The typical way we'd handle this is with printOnFailure.
| printPath: false, | ||
| printPlatform: false, | ||
| var useCompactReporter = const bool.fromEnvironment( | ||
| 'test_core.compactReporter', |
There was a problem hiding this comment.
I think it would be better to match the args format for consistency. -Dtest_core.reporter=compact
We can't import the json reporter, but I don't see any reason not to support at least the failures-only reporter (which could be useful) and the github reporter (which is trivial to support) from the start.
| } | ||
| } | ||
|
|
||
| List<Pattern> _generateExpectedOutput({ |
There was a problem hiding this comment.
[optional] I don't think we need to verify every character of the output. It might be easier to test that the compact output does contain a carriage return and the expanded output doesn't. We already test the particular details of the reporters in other tests, we just need to test for any sign that the config is plumbed through correctly.
| @@ -1,3 +1,6 @@ | |||
| ## 0.6.14 | |||
| * Support environment flags for configuring the global declarer. | |||
There was a problem hiding this comment.
This reads like we support reading from Platform.environment.
I wonder, should we be trying to do that instead? For the VM case it's probably little consequence either way, for AOT native we'd probably prefer Platform.environment support, and for web we'd prefer what's implemented.
Would there be value in supporting a Platform.environment fallback in places where we can import it? I'd be happy to write up the conditional import version if we think it's useful.
+1 to this - it's likely that the direct-run use case will continue to have a long-tail of UX rough edges and it's unlikely we'll prioritize that as a first class route for running tests. That said, I don't thinks I see a significant downside to reading from either
I don't think I share this concern, although it maybe pushes me a little towards taking back my suggestion about supporting |
4 participants
The global declarer is used by package:test_reflective_loader which has no means of configuring the test output. It is for instance used to run all tests in package:analyzer, and the current output prints a line for each test (of which there is currently >32000) and uses colors in the output. This makes it very hard to output to find failing tests, since the failures are hidden within all the succeeding tests and piping the output to a file for easier search still leaves the color in the output.
This change adds .fromEnvironment variables to the creation of the global declarer, such that it can be configured at the command line. This allow for enabling compact output which only displays the failing tests, and whether to use colors in the output.