-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Some tests no longer work with custom local builds #56041
Comments
Labels: |
If you do:
Then the test passes just fine. :) |
0dbeb21 is the only change to this part of test_runner in that time (from @nshahan), but it's not at all clear that that's the cause. Presumably --named-configuration is now required? If so, I'm not sure what I'm supposed to pass. the custom-configuration-1 appears to be generated via: sdk/pkg/test_runner/lib/src/options.dart Line 787 in 40661a5
Which is only hit if namedConfigurations (earlier in the file) is empty. Maybe my test.py line is just an old style? https://github.com/dart-lang/sdk/blob/main/docs/Testing-the-VM.md doesn't seem to cover this case. |
Example vm tests which fail this way:
|
I'm not sure of the priority of this (blocking something?), but a |
True, git-bisect would. We've skipped the tests in our fork (or maybe someone will explain to us how we're supposed to run a non-named config?) so it's not high priority from our side. |
Trying:
Where test.sh is:
|
|
@eseidel That CL should not influence how tests are run and cause
|
I'm 100% certain it's that change. :) Unclear if I'm using test.py correctly, but the command line above works before that change and breaks after. sdk/pkg/expect/lib/config.dart Line 18 in 0db3b12
The configuration is attempted to parse, and it fails to figure out what the compiler is from "custom-configuration-1". Before ca9eb4a it didn't try to figure out the compiler/runtime from the configuration string, rather from Platform.executable. |
|
It also may be a inside-google vs. outside thing. I don't know how the "named configurations" are determined, nor do I understand yet what about my test.py invocation which throws us off the named configuration path and into the "custom configuration" path where this hits 🤷 |
|
Ah right, my bad. I somehow interpreted this as the test runner failing, instead of the test failing. 😴 ☕ Only relying on the configuration string seems to be bad. I think |
This unfortunately doesn't seem to have fixed things. If I try on main: (09e87e6)
It still immediately fails:
|
I don't have powers on this repo (including re-opening bugs, but I can file a new one if needed). |
Ah, you're also trying to run as |
@eseidel https://dart-review.googlesource.com/c/sdk/+/372360 should fix Any other custom configurations you're using? |
I'm not trying to swim upstream at all. :) If there is a better way I should be trying to run tests I'm happy to do that. |
It seems that (some) of our infra is rather configuration-name-dependent. Maybe we should be aiming on making a named configuration for every combination of flags? I'm not sure if that is even feasible. Thoughts @athomas @rmacnak-google? |
It does feel like the left and and right hand aren't talking to each other here. It's the test runner that is producing this configuration name, and then later another part of the test system which is assuming it's in a specific format. This presumably works inside Google because you're using "named" configurations? But I'm not sure how to set that up as someone who has made any modifications to the VM (also all my test cases above, are with unmodified VM checkouts from main, just changing what flags I do/don't pass to test.py). Happy to follow whatever rules, would just like them written down somewhere. :) e.g. https://github.com/dart-lang/sdk/blob/main/docs/Testing-the-VM.md |
Thanks. I don't think we're even using that config, I just was trying to run "test.py" on main (without any flags) and hit that since I was still seeing failures on our bots after cherry picking your change from last night (thank you!) over to our fork this morning. We use: |
Arbitrary VM flags can be part of combination of flags, and I don't think we want to start encoding all that into configuration names. It seems most of these uses of the configuration name are tests that want to be skipped for JIT, so maybe moving AOT-only tests to a separate directory and using the status files to skip them mostly handles this. |
Some tests no longer work with custom configurations.
These used to work back in 3.19.x (3.3.x) Dart and broke with 3.4.x. (Edit: broke at ca9eb4a)
And you will get:
The text was updated successfully, but these errors were encountered: