Conversation
flutter-dashboard
bot
added
the
c: contributor-productivity
| ] | ||
| shard: web_tool_tests | ||
| subshard: web | ||
| subshard: "1_2" |
There was a problem hiding this comment.
uh oh, is this going to work? I'm guessing the test runner is expecting this value to be "web"?
There was a problem hiding this comment.
Looking at this logs https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8801824234378758625/+/u/run_test.dart_for_web_tool_tests_shard_and_subshard_web/test_stdout from a previous run I see Invalid subshard name "web". Expected format "[int]_[int]" ex. "1_3" So I think is the other way around; for linux and mac it expects I think 1_1 but since it fails it is running it in a single shard anyways. I can make the change to linux and mac to be 1_1 and avoid that error.
Edit: that is happening because now I am using _selectIndexOfTotalSubshard which expect subshard keys
There was a problem hiding this comment.
lol, good catch. i think it's expecting an empty subshard if there's only one. i wonder why we don't fail the build on an invalid subshard value. Anyway, looks good.
There was a problem hiding this comment.
I agree I think we should fail if invalid subshard name, I spent some time debugging why my shard was running integration tests and turns out that if you make this mistake _selectIndexOfTotalSubshard returns an empty array which then on _runDartTest if you pass an empty array you end up with a command like this run test --flags and then it runs every single test on your workingDirectory. I will make an issue to fix this
| ] | ||
| shard: web_tool_tests | ||
| subshard: web | ||
| subshard: "1_1" |
There was a problem hiding this comment.
Target name Linux web_tool_tests => Linux web_tool_tests_1_2.
Same for the Mac one.
Seems the Windows one has been updated.
There was a problem hiding this comment.
Linux web_tool_tests_1_1 right?
There was a problem hiding this comment.
Oh, this only has one shard. Then it should be good.
There was a problem hiding this comment.
question: should we just leave this field empty then?
There was a problem hiding this comment.
From a practical perspective it's fine to remove it
if (subshardName == null) {
print('$kSubshardKey environment variable is missing, skipping sharding');
return tests;
}
There was a problem hiding this comment.
Either LGTM. I don't have strong preference on it.
There was a problem hiding this comment.
But then again if in the future we want to further subshard linux/mac I think it will help to remember that this are already "subshard" ready?
| ] | ||
| shard: web_tool_tests | ||
| subshard: web | ||
| subshard: "1_1" |
There was a problem hiding this comment.
Oh, this only has one shard. Then it should be good.
|
/cc @CaseyHillers this may need a CP from release branches. |
Jasguerrero
added
the
autosubmit
Thanks, since this doesn't change any existing target names it looks fine. I'll monitor when we ship the next release to make sure it didn't cause any breakages. |
|
Ah, I missed that. We'll likely ack this as not running since it's covered on other platforms |
4 participants
Fixes: #112440
Pre-launch Checklist
///).