-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Bug 1613483 - Add all Browsertime tests with visual metrics to Fenix repo. #9087
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9087 +/- ##
============================================
- Coverage 19.54% 19.53% -0.01%
Complexity 511 511
============================================
Files 329 329
Lines 13136 13136
Branches 1741 1741
============================================
- Hits 2567 2566 -1
Misses 10345 10345
- Partials 224 225 +1
Continue to review full report at Codecov.
|
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 great to me overall! Thank you very much for do this huge amount of work! I know it's not easy to reach this point, but you did!
I see some great things in the PR and few that would need to be improved. Please let me know what you think about the points below. I'll be happy to review the PR again 🙂
The commit |
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.
Looks good to me! Thanks for porting these tests over 😃
group_attr = None | ||
if only_attributes: | ||
for attr in only_attributes: | ||
if attr in task.attributes: | ||
group_attr = attr | ||
break | ||
if not group_attr: | ||
continue |
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.
Nit: I think we can simplify the logic with these 3 lines.
group_attr = None | |
if only_attributes: | |
for attr in only_attributes: | |
if attr in task.attributes: | |
group_attr = attr | |
break | |
if not group_attr: | |
continue | |
if only_attributes: | |
if not any(attr in task.attributes for attr in only_attributes): | |
continue |
@JohanLorenzo I've removed the fetch-content script from the docker and ran a full set of tests and they are going well. Would you be able to merge this? |
👍 I ran https://hg.mozilla.org/build/braindump/file/tip/taskcluster/taskgraph-diff/ to be sure the existing graphs aren't changed in an unexpected way. I saw the toolchain and the docker image tasks added, to them, which is good. Thus, here we go! |
Great, thanks for the reviews and merge @JohanLorenzo! :) |
…repo. (mozilla-mobile#9087) * Add visual-metrics docker type. * Add required browsertime toolchain fetches. * Add browsertime tests for technical and visual metrics. * Run browsertime tests in a cron task. * Run visual metrics on all browsertime tests. * Use spaces instead of tabs, and resolve visual-metric nits. * Enable browsertime on pull request for testing. * Restrict PR tests to amazon on browsertime. * First attempt using multi_dep. * Add a primary dependency to browsertime. * Try by not popping. * Debug prints. * Make one grouping per browsertime task. * Try without the multi_dep transform. * Delete dependent-tasks in visual-metrics transformer. * Update setuptools installed and copy run-on-tasks-for. * Use get when getting run-on-tasks-for. * Add new pinned requirements. * Try it. * Set run-on-tasks-for properly. * Remove print statement. * Remove single_dep loader, and print statements. * Remove run-on-tasks-for testing setting. * Restart testing, and set user to root in visual-metrics Docker. * Remove testing settings. * Remove fetch-content from Docker. * Change attributes grouping method. * Run all tests as a check. * Undo testing changes, and fix a bad test name.
This pull request adds Raptor-Browsertime tests to the tests run on the Fenix-repo branch.
To be able to run Raptor-Browsertime on the Fenix branch, a few things had to be changed.
single_dep.py
loader is added to handle theonly-for-attributes
setting that the visual-metrics tasks depend on.In preparation for a full migration, these tests will run for 1 week, and if everything works fine, the Raptor-Webext tests will be disabled and these will be promoted.