-
Notifications
You must be signed in to change notification settings - Fork 27.1k
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
Enable linux and win hostonly devicelab tests. #66085
Enable linux and win hostonly devicelab tests. #66085
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
All the hostonly_devicelab tests are now running separately. Linux and mac are all passing, I kept the ones failing in windows to continue running them on cirrus while we fix them. |
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.
It looks like you ran dartfmt on this dev/bots/test.dart
, if you revert that it would make the diff smaller and easier to review. It would also make a regression easier to spot. The framework intentionally does not use dartfmt
, see https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#formatting
This PR moves most of the hostonly devicelab tests to luci except for 6 that are still failing in LUCI. Bug: flutter#63242
d2f93d3
to
d30a886
Compare
Reverted back the formatting. I'll check if I still get the error missing {} after ifs. |
looks like analysis passed :) |
.cirrus.yml
Outdated
@@ -238,7 +238,7 @@ task: | |||
<<: *FIREBASE_SHARD_TEMPLATE | |||
|
|||
- name: web_smoke_test | |||
only_if: "changesInclude('.cirrus.yml', 'examples/hello_world/**' ,'dev/**', 'packages/flutter/**', 'packages/flutter_test/**', 'packages/flutter_tools/lib/src/test/**', 'packages/flutter_web_plugins/**', 'bin/**') || $CIRRUS_PR == ''" | |||
only_if: "changesInclude('.cirrus.yml', 'examples/hello_world/**' ,'dev/**', 'packages/flutter/**', 'packages/flutter_test/**', 'packages/flutter_tools/lib/src/test/**', 'packages/flutter_web_plugins/**', 'bin/**') || $CIRRUS_PR != ''" |
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.
these diffs are always so hard for me to read...
So this is saying we always want to run the test for any PR, and on post-submit only on affected diffs...right?
This doesn't seem like the right logic. Should it be && $CIRRUS_PR == ''
, which would be only run pre-submit with a diff on the affected files?
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.
I see, yeah I think it has to be && but I thought $CIRRUS_PR == '' meant postsubmit.
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.
lol, yeah you're right. so && $CIRRUS_PR != ''
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.
This is why I put question marks at the end of all my statements, I never get this right the first try.
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.
yeah, errors in those conditionals are usually difficult to spot. Updated.
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.
LGTM
Description
This PR moves most of the hostonly devicelab tests to luci except for 6
that are still failing in LUCI.
Related Issues
Bug:
#63242
Tests
N/A
Checklist
Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.