Added an option to allow submitting test-perf result to perfherder #14147

shinglyu · 2016-11-09T07:02:09Z

This patch enables us to run ./mach test-perf --submit in CI to submit the result to perfherder. r? @aneeshusa

./mach build -d does not report any errors
./mach test-tidy does not report any errors
[] These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because too many manual setup required to test it

This change is

highfive · 2016-11-09T07:02:12Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/servo/testing_commands.py
@aneeshusa: etc/ci/performance/README.md, etc/ci/performance/test_all.sh, etc/ci/performance/test_perf.sh

wafflespeanut · 2016-11-14T09:10:41Z

etc/ci/performance/README.md

@@ -7,27 +7,21 @@ Servo Page Load Time Test

 # Basic Usage

-## Prepare the test runner
+If you want to run a performance test for your servo build. Simply go to the top-level servo directory and run `./mach test-perf`. The test result JOSN will be saved to `etc/ci/performance/output/`. You can compare two test results with the `test_differ.py` script. Run `python test_differ.py -h` for instructions.


Nit: Fixing style,

"./mach test-perf can be used to run a performance test on your servo build. The test result JSON will be saved to etc/ci/performance/output/. You can then run python test_differ.py to compare these two test results. Run python test_differ.py -h for instructions."

wafflespeanut · 2016-11-14T09:10:44Z

etc/ci/performance/README.md

-* Run `prepare_manifest.sh` to transform the tp5n manifest to our format
-* Install the Python3 `treeherder-client` package. For example, to install it in a virtualenv: `python3 -m virtualenv venv; source venv/bin/activate; pip install "treeherder-client>=3.0.0"`
+# Setup for CI machine
+


What should be done here?

This H1 is for the subsections "For Servo" and "For Gecko", I'll change them to "CI for Servo" to make it clear.

Oh, never mind 😄

But yeah, it could just be "CI for Servo"

aneeshusa

Just a few nits, looks like this will be useful!

aneeshusa · 2016-11-15T06:08:59Z

etc/ci/performance/test_perf.sh

@@ -34,4 +34,9 @@ pip install "treeherder-client>=3.0.0"
 mkdir -p servo
 mkdir -p output
 ./git_log_to_json.sh > servo/revision.json && \
-./test_all.sh --servo
+
+if [[ "${1}" = "--submit" ]]; then


You need to guard this by checking the number of arguments before looking at any of their values.

aneeshusa · 2016-11-15T06:10:34Z

etc/ci/performance/test_perf.sh

@@ -34,4 +34,9 @@ pip install "treeherder-client>=3.0.0"
 mkdir -p servo
 mkdir -p output
 ./git_log_to_json.sh > servo/revision.json && \


No need for the && \ at the end.

aneeshusa · 2016-11-15T06:11:03Z

etc/ci/performance/test_perf.sh

@@ -34,4 +34,9 @@ pip install "treeherder-client>=3.0.0"
 mkdir -p servo
 mkdir -p output


Why do we need to make this output dir? Add a comment or remove it.

aneeshusa · 2016-11-15T06:12:37Z

python/servo/testing_commands.py

-        return call(["bash", "test_perf.sh"],
+        cmd = ["bash", "test_perf.sh"]
+        if submit:
+            cmd += ["--submit"]


We should check that the Treeherder environment variables are set if submit is True before we actually run the command, to improve the user experience (e.g. if you don't read carefully).

wafflespeanut · 2016-11-17T15:00:32Z

./etc/ci/performance/test_perf.sh:38: variable substitutions should use the full "${VAR}" form

wafflespeanut · 2016-11-26T15:34:29Z

@aneeshusa This is waiting on your review.

shinglyu · 2016-11-28T09:06:00Z

@aneeshusa Since there is no major issue, and I need to land this ASAP, I'll ask @larsbergstrom for the final sign-off. Hope you don't mind!

larsbergstrom · 2016-11-29T03:00:58Z

@bors-servo r=larsbergstrom,aneeshusa

We can chat more in Hawaii if there are any follow-ups needed here :-)

bors-servo · 2016-11-29T03:00:59Z

📌 Commit 4f377ac has been approved by larsbergstrom,aneeshusa

bors-servo · 2016-11-29T03:29:43Z

⌛ Testing commit 4f377ac with merge 90e77f9...

@aneeshusa

…eshusa Added an option to allow submitting test-perf result to perfherder  This patch enables us to run `./mach test-perf --submit` in CI to submit the result to perfherder. r? @aneeshusa ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because too many manual setup required to test it   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14147)

bors-servo · 2016-11-29T04:22:03Z

💔 Test failed - linux-rel-wpt

bors-servo · 2016-11-29T05:22:55Z

☔ The latest upstream changes (presumably #14194) made this pull request unmergeable. Please resolve the merge conflicts.

shinglyu · 2016-12-01T02:30:36Z

The conflict is in the README file, so no big deal. The test seems to be some network error while running filter-intermittents

shinglyu · 2016-12-01T02:33:24Z

@bors-servo r=larsbergstrom,aneeshusa

bors-servo · 2016-12-01T02:33:25Z

📌 Commit ddd0322 has been approved by larsbergstrom,aneeshusa

bors-servo · 2016-12-01T03:11:17Z

⌛ Testing commit ddd0322 with merge 7d66bd7...

@aneeshusa

…eshusa Added an option to allow submitting test-perf result to perfherder  This patch enables us to run `./mach test-perf --submit` in CI to submit the result to perfherder. r? @aneeshusa ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because too many manual setup required to test it   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14147)

bors-servo · 2016-12-01T04:12:26Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

highfive assigned aneeshusa Nov 9, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 9, 2016

shinglyu mentioned this pull request Nov 9, 2016

Run the page load performance test in CI #12988

Closed

wafflespeanut reviewed Nov 14, 2016

View reviewed changes

aneeshusa suggested changes Nov 15, 2016

View reviewed changes

aneeshusa added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 15, 2016

shinglyu force-pushed the test-perf-submit branch from c052aa1 to 0b59b8a Compare November 16, 2016 06:49

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 16, 2016

shinglyu force-pushed the test-perf-submit branch from 0b59b8a to 4f377ac Compare November 21, 2016 06:23

highfive assigned larsbergstrom and unassigned aneeshusa Nov 29, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 29, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 29, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Nov 29, 2016

Add an option to submit test-perf result to perfherder

ddd0322

shinglyu force-pushed the test-perf-submit branch from 4f377ac to ddd0322 Compare December 1, 2016 02:23

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Dec 1, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Dec 1, 2016

bors-servo merged commit ddd0322 into servo:master Dec 1, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Dec 1, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added an option to allow submitting test-perf result to perfherder #14147

Added an option to allow submitting test-perf result to perfherder #14147

shinglyu commented Nov 9, 2016 •

edited by larsbergstrom

highfive commented Nov 9, 2016

wafflespeanut Nov 14, 2016

wafflespeanut Nov 14, 2016

shinglyu Nov 16, 2016

wafflespeanut Nov 16, 2016

wafflespeanut Nov 16, 2016

aneeshusa left a comment

aneeshusa Nov 15, 2016

aneeshusa Nov 15, 2016

aneeshusa Nov 15, 2016

aneeshusa Nov 15, 2016

wafflespeanut commented Nov 17, 2016

wafflespeanut commented Nov 26, 2016

shinglyu commented Nov 28, 2016

larsbergstrom commented Nov 29, 2016

bors-servo commented Nov 29, 2016

bors-servo commented Nov 29, 2016

bors-servo commented Nov 29, 2016

bors-servo commented Nov 29, 2016

shinglyu commented Dec 1, 2016

shinglyu commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

		@@ -34,4 +34,9 @@ pip install "treeherder-client>=3.0.0"
		mkdir -p servo
		mkdir -p output

Added an option to allow submitting test-perf result to perfherder #14147

Added an option to allow submitting test-perf result to perfherder #14147

Conversation

shinglyu commented Nov 9, 2016 • edited by larsbergstrom

highfive commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aneeshusa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wafflespeanut commented Nov 17, 2016

wafflespeanut commented Nov 26, 2016

shinglyu commented Nov 28, 2016

larsbergstrom commented Nov 29, 2016

bors-servo commented Nov 29, 2016

bors-servo commented Nov 29, 2016

bors-servo commented Nov 29, 2016

bors-servo commented Nov 29, 2016

shinglyu commented Dec 1, 2016

shinglyu commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

shinglyu commented Nov 9, 2016 •

edited by larsbergstrom