Closed Bug 1378139 Opened 3 years ago Closed 3 years ago

Add a stylo-only talos test suite for perf-reftest-singletons

Categories

(Testing :: Talos, enhancement)

Version 3
enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: rwood, Assigned: rwood)

References

(Blocks 1 open bug)

Details

(Whiteboard: [stylo])

Attachments

(3 files)

In Bug 1363104 we changed the talos 'perf-reftest' suite and bloom_basic test so that the test measures both bloom_basic and bloom_basic_ref and reports the comparison of those two, instead of reporting separate raw values for each.

Now for stylo only, we want a new talos test suite that will run just bloom_basic on it's own and report only the raw values of that test. This bug is for the talos additions for that. There will be a separate bug filed for the corresponding buildbot/treeherder config changes required.
(In reply to Robert Wood [:rwood] from comment #0)
> In Bug 1363104 we changed the talos 'perf-reftest' suite and bloom_basic
> test so that the test measures both bloom_basic and bloom_basic_ref and
> reports the comparison of those two, instead of reporting separate raw
> values for each.
> 
> Now for stylo only, we want a new talos test suite that will run just
> bloom_basic on it's own

Ideally, the suite would be extensible enough that we could add a few other things in addition to bloom_basic without much trouble.
In reply to Bobby Holley (:bholley) from https://bugzilla.mozilla.org/show_bug.cgi?id=1363104#c32

Ok thanks, I will use the updated 'bloom_basic.html' from [1]. For the suite name I like 'perf-reftest-singletons' or maybe 'perf-reftest-absolutes'.

[1] https://github.com/heycam/style-perf-tests/tree/master/perf-reftest
Sounds great, thanks!
Summary: Add a stylo-only talos test suite for bloom_basic raw values → Add a stylo-only talos test suite for perf-reftest-singletons
Blocks: 1378451
Whiteboard: [stylo]
Comment on attachment 8883596 [details]
Bug 1378139 - Add stylo-only talos test suite for perf-reftest singletons;

https://reviewboard.mozilla.org/r/154534/#review160200

Tested locally with success, with mach talos-tests and in standalone mode.
Code looks good and easy to follow.
Attachment #8883596 - Flags: review?(ionut.goldan) → review+
Hey :bholley,

I grabbed your latest bloom_basic.html and util.js from the repo in comment 2. When landing these on try there are a bunch of ES lint errors [1].

Would you mind addressing these in your repo? Otherwise everytime I grab an updated version from your repo I will need to re-fix them. I can sync your repo and fix them if you like, however I'm not sure how to address the 'xxxx not defined' errors.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=738c824a91bf62ca280f9018ca4de5546ba06580&selectedJob=112557492

Thanks!
Flags: needinfo?(bobbyholley)
Blocks: 1379164
(In reply to Robert Wood [:rwood] from comment #7)
> Hey :bholley,
> 
> I grabbed your latest bloom_basic.html and util.js from the repo in comment
> 2. When landing these on try there are a bunch of ES lint errors [1].
> 
> Would you mind addressing these in your repo? Otherwise everytime I grab an
> updated version from your repo I will need to re-fix them. I can sync your
> repo and fix them if you like, however I'm not sure how to address the 'xxxx
> not defined' errors.

Hm, I'm not sure how the linter works, but those 'not defined' things are defined in util.rs...

Fastest is probably for you to figure out what's going on there and then PR any changes you need to the github repo (which is managed by heycam). He's quite responsive on IRC if you need a quick merge.
Flags: needinfo?(bobbyholley)
PR in style-perf-tests to fix bloom-basic and util.js eslint errors
Attachment #8884382 - Flags: review?(cam)
Pushed by rwood@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7022e032fb01
Add stylo-only talos test suite for perf-reftest singletons; r=igoldan
Hey :bholley, the perf-reftest-singletons will always use the same *.html test files as the /perf-reftest (comparison) tests will correct?

I have the /perf-reftest-singleton manifest pointing to /perf-reftest/bloom_basic.html which makes sense to me, to not have two copies of the same test. I uploaded the latest bloom_basic.html and utils.js to /perf-reftest-singletons though - which I realize is no point. So I'll add another patch to update bloom_basic in perf-reftest to the latest instead if you agree.
Flags: needinfo?(bobbyholley)
https://hg.mozilla.org/mozilla-central/rev/7022e032fb01
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(In reply to Robert Wood [:rwood] from comment #14)
> Hey :bholley, the perf-reftest-singletons will always use the same *.html
> test files as the /perf-reftest (comparison) tests will correct?
> 
> I have the /perf-reftest-singleton manifest pointing to
> /perf-reftest/bloom_basic.html which makes sense to me, to not have two
> copies of the same test. I uploaded the latest bloom_basic.html and utils.js
> to /perf-reftest-singletons though - which I realize is no point. So I'll
> add another patch to update bloom_basic in perf-reftest to the latest
> instead if you agree.

I think it makes sense to keep the actual tests separate. The fact that bloom-basic currently is useful as both a perf-reftest and as a singleton is a bit of an outlier, and the other two singleton tests that I added don't make sense as perf-reftests. Moreover, it may end up useful to tweak the singleton version of bloom-basic with different parameters (to make it less noisy).

That said, we should share util.js if at all possible.
Flags: needinfo?(bobbyholley)
Ok thanks for the info! Reopening, to fix the manifest to point to the new singleton bloom_basic, not the one in /perf-reftest
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8884954 - Flags: review?(ionut.goldan)
Comment on attachment 8884954 [details] [diff] [review]
fixmanifest.patch

Again, the actual test ran OK. I also ran | mach lint --linter eslint --quiet |, which didn't report any issues.
Attachment #8884954 - Flags: review?(ionut.goldan) → review+
https://hg.mozilla.org/mozilla-central/rev/16d2d4153a69
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.