Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Run Flutter framework tests against the web engine in Cirrus #16343

Merged
merged 4 commits into from
Feb 7, 2020
Merged

Run Flutter framework tests against the web engine in Cirrus #16343

merged 4 commits into from
Feb 7, 2020

Conversation

mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Feb 3, 2020

I got bit multiple times because flutter tests currently don't run against the web engine in Cirrus (#16268, #14569).

These sort of mistakes could potentially be avoided too:

@mdebbar mdebbar added the platform-web Code specifically for the web engine label Feb 3, 2020
@mdebbar mdebbar requested review from yjbanov and nturgut February 3, 2020 21:37
@mdebbar mdebbar self-assigned this Feb 3, 2020
nturgut
nturgut reviewed Feb 3, 2020
View reviewed changes
.cirrus.yml Outdated
@@ -80,6 +80,10 @@ task:
test_framework_script: |
cd $FRAMEWORK_PATH/flutter/packages/flutter
../../bin/flutter test --local-engine=host_debug_unopt
# Run Flutter framework tests against the web engine.
test_framework_web_script: |
cd $FRAMEWORK_PATH/flutter/packages/flutter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we add this to LUCI instead since Godofredo is about to finish LUCI recipes :)

I think Linux Chrome already runs!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I spoke with @godofredoc and for the time-being, we are continuing to use Cirrus until the end of Q1. So I think it's still worth adding these tests to Cirrus now.

@mdebbar mdebbar force-pushed the cirrus_web_tests branch 5 times, most recently from d582777 to 53e9504 Compare February 4, 2020 23:53
@mdebbar mdebbar marked this pull request as ready for review February 5, 2020 02:24
@mdebbar mdebbar requested review from nturgut and godofredoc February 5, 2020 02:25
yjbanov
yjbanov approved these changes Feb 6, 2020
View reviewed changes
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

This change also highlights a problem with our infra: when we shard tests, each shard still needs to duplicate work, such as ninja -C out/host_debug_unopt. That is in addition to the manual work that goes into configuring the shards. As far as I understand, LUCI is not fixing this problem. Sharding is still manual, and there's no build artifact sharing.

/cc @godofredoc @Hixie

@godofredoc
Copy link
Contributor

Actually LUCI solves the duplication of work, it basically builds once, archive the artifacts to isolated and then multiple tasks can be triggered to execute the tests using the same build artifacts.

@yjbanov
Copy link
Contributor

yjbanov commented Feb 6, 2020 •
edited
Loading

Actually LUCI solves the duplication of work, it basically builds once, archive the artifacts to isolated and then multiple tasks can be triggered to execute the tests using the same build artifacts.

Does it happen automatically? If you look at this PR's LUCI builds, this and this, the "build host_debug_unopt" was fully executed on both builds, taking 4-5 minutes. So perhaps LUCI can do it, but it doesn't for us.

@godofredoc
Copy link
Contributor

Actually LUCI solves the duplication of work, it basically builds once, archive the artifacts to isolated and then multiple tasks can be triggered to execute the tests using the same build artifacts.

Does it happen automatically? If you look at this PR's LUCI builds, this and this, the "build host_debug_unopt" was fully executed on both builds, taking 4-5 minutes. So perhaps LUCI can do it, but it doesn't for us.

That is correct luci can optimize to that level but we need to tell it to do for us. For example for linux web engine we are planning to build host_debug_unopt once, archive to isolated and then trigger two tasks one for firefox and one for chrome using the same artifacts.

For post submit we can optimize even more building once, archiving to gcs and then sharding all the tests using the gcs artifacts.

@mdebbar
Copy link
Contributor Author

mdebbar commented Feb 6, 2020

@godofredoc this PR adds 8 new tasks to Cirrus. Are there any concerns regarding our Cirrus capacity?

@yjbanov
Copy link
Contributor

yjbanov commented Feb 6, 2020

@godofredoc this PR adds 8 new tasks to Cirrus. Are there any concerns regarding our Cirrus capacity?

Since these are the exact same tests we already run on every PR in flutter/flutter, this should only add engine commit rate to the existing commit rate, so I expect the incremental cost is 10%-20%.

nturgut
nturgut approved these changes Feb 6, 2020
View reviewed changes
Copy link
Contributor

@nturgut nturgut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a way of creating task template in cirrus ci which might make this file a lot shorter: https://cirrus-ci.org/guide/tips-and-tricks/

Up to you, I never used it. I just saw it on their site.

@mdebbar mdebbar force-pushed the cirrus_web_tests branch 2 times, most recently from 9b1830d to 5b1c35d Compare February 6, 2020 21:41
@mdebbar mdebbar mentioned this pull request Feb 6, 2020
Merged
13 tasks
@mdebbar mdebbar merged commit 557f3a2 into flutter:master Feb 7, 2020
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 7, 2020
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 7, 2020
@engine-flutter-autoroll
557f3a2 Run Flutter framework tests against the web engine in Cirrus (f…
7415561 
…lutter/engine#16343)
dnfield pushed a commit to flutter/flutter that referenced this pull request Feb 7, 2020
@engine-flutter-autoroll
Roll engine f3ce90e..580503c (24 commits) (#50363)
ef74572 
* 71ec0a1 Relax timing restrictions on WakeUpTimersAreSingletons. (flutter/engine#16446)

* 3ac1e6d Disable unit tests using --gtest-filter instead of at compile time (flutter/engine#16472)

* 80be2c4 Fix RasterCache LRU logic, opportunistic simplifications. (flutter/engine#16434)

* ca40f11 Roll src/third_party/skia 7f36405ea3ec..c0360582d211 (6 commits) (flutter/engine#16476)

* c9e7713 Fix analyzer warnings for frontend_server change (flutter/engine#16470)

* 9ada1b0 Fix elf_loader.cc on Fuchsia, add a TODO for proper fix

* 0517627 Enable runtime_unittests on Fuchsia

* 03f639e Add noexcept annotations to EnableValue moves (flutter/engine#16478)

* 00904dd Various fixes in CanvasKit (flutter/engine#16433)

* de7022b Roll src/third_party/skia c0360582d211..121750c2efff (7 commits) (flutter/engine#16479)

* d2aab27 Enable shell_unittests on Fuchsia with Vulkan dependencies. (flutter/engine#16376)

* eec73e3 Roll src/third_party/dart b3396cbdcae1..49850e6919f7 (45 commits) (flutter/engine#16480)

* 557f3a2 Run Flutter framework tests against the web engine in Cirrus (flutter/engine#16343)

* 477527b Roll src/third_party/skia 121750c2efff..046f9893b953 (4 commits) (flutter/engine#16482)

* eb8691f Code cleanup on destructors (flutter/engine#16481)

* 7397817 Roll fuchsia/sdk/core/linux-amd64 from A9STP... to g2s3c... (flutter/engine#16484)

* 313527d Roll src/third_party/dart 49850e6919f7..16782e6c171f (16 commits) (flutter/engine#16485)

* 83feaf4 Roll src/third_party/skia 046f9893b953..97bf6578796c (1 commits) (flutter/engine#16486)

* 9184058 Roll src/third_party/skia 97bf6578796c..f3560b680e35 (1 commits) (flutter/engine#16487)

* 0db017d Roll src/third_party/dart 16782e6c171f..d765d237460d (1 commits) (flutter/engine#16488)

* f9ed07c Roll fuchsia/sdk/core/linux-amd64 from g2s3c... to LvSlH... (flutter/engine#16489)

* a1b91da Roll src/third_party/skia f3560b680e35..77fdf66946d2 (1 commits) (flutter/engine#16490)

* 7edb803 Roll src/third_party/dart d765d237460d..514a8d4c8417 (7 commits) (flutter/engine#16491)

* 580503c Roll src/third_party/skia 77fdf66946d2..87e3bef6f82f (2 commits) (flutter/engine#16492)
NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
@mdebbar @NoamDev
Run Flutter framework tests against the web engine in Cirrus (flutter…
9d579ce 
…#16343)
NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
@NoamDev
Revert "Run Flutter framework tests against the web engine in Cirrus (f…
9e33bd8 
…lutter#16343)"

This reverts commit 9d579ce.
@mdebbar mdebbar deleted the cirrus_web_tests branch April 15, 2021 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nturgut nturgut nturgut approved these changes

@yjbanov yjbanov yjbanov approved these changes

@godofredoc godofredoc Awaiting requested review from godofredoc

Assignees

@mdebbar mdebbar

Labels
cla: yes platform-web Code specifically for the web engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mdebbar @godofredoc @yjbanov @nturgut @googlebot