Check if MANIFEST.json changes as per #8587 #8601

mfeckie · 2015-11-19T11:28:44Z

Adds a script which checks if either of the MANIFEST.json files change in test/wpt in response to #8587

Addresses currently incorrect MANIFEST.json
Adds checking script

PR for saltfs to add to CI checks to follow (servo/saltfs#163)

https://reviewable.io/reviews/servo/servo/8601


        Commit autogenerated manifest for baseline position


        Add script to check if MANIFEST.json files change


        Add script to check if MANIFEST.json is changes in wpt

highfive · 2015-11-19T11:28:51Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

Manishearth · 2015-11-19T14:40:10Z

etc/ci/manifest_changed.sh

@@ -0,0 +1,4 @@
+#!/bin/bash
+diff=$(git diff -- tests/wpt/**/MANIFEST.json)


why the double *?

jdm · 2015-11-19T16:40:45Z

This doesn't actually run the manifest update command, does it?

mfeckie · 2015-11-19T21:34:02Z

@jdm it doesn't run update. I ran the manifest update when running the tests initially to resolve the differences you pointed to the likely came from hand editing of the MANIFEST files. I committed the changes to ensure baseline is correct. Does that make sense? If it's an incorrect approach I can back out that commit.

jdm · 2015-11-19T22:27:04Z

The changes here are correct, but the new script will never report any violations unless we explicitly update the manifest each time it's run. Unlike the check for Cargo.lock files, we don't automatically update the manifest during the build.

mfeckie · 2015-11-19T23:23:05Z

@jdm OK, so would auto running the update be an additional step for this PR? If so, can you point me to the place to start looking 😄

jdm · 2015-11-20T05:28:22Z

It's a step we need to perform in the script right before checking the result of git diff.

mfeckie · 2015-11-20T05:40:50Z

So, run all the tests? Or is there method to just update the manifest?

jdm · 2015-11-20T14:48:15Z

Oh, hrm. That's a good point. @jgraham is there a way to only trigger the manifest update in isolation?

eefriedman · 2015-11-20T20:09:00Z

./mach test-wpt --manifest-update SKIP_TESTS?

jgraham · 2015-11-20T22:11:25Z

Something like ./manifest -r -p ../metadata/MANIFEST.json in tests/wpt/web-platform-tests will regenerate the manifest. You would also need to do the same for the mozilla tests passing in --url-base=/_mozilla/ and the relevant paths under tests/wpt/mozilla, and similarly for the CSS tests.

mfeckie · 2015-11-20T22:14:44Z

@jgraham Is there any reason not to use the suggestion of ./mach test-wpt --manifest-update SKIP_TESTS by @eefriedman ?


        Run manifest-update without running tests prior to checking diff

mfeckie · 2015-11-20T22:55:55Z

etc/ci/manifest_changed.sh, line 2 [r3] (raw file):
Will fix, it was because I was using ZSH.

Comments from the review on Reviewable.io


        Only use single * and ignore mach output

jdm · 2015-11-20T23:19:11Z

Let's run the same for ./mach test-css too, which is the other MANIFEST.json under tests/wpt/metadata-css.


        Also run checks for test-css MANIFEST.json

mfeckie · 2015-11-20T23:36:02Z

@jdm Added. Running the two commands isn't quick, just want to flag in case it's an issue in terms of test run time.

jdm · 2015-11-21T04:32:50Z

Oh, one last request! We can add the new script to http://mxr.mozilla.org/servo/source/.travis.yml#13 as well for maximum coverage.

jdm · 2015-11-21T04:32:58Z

(Sorry to keep changing the goalposts :)


        Add to manifest checks to .travis.yml

mfeckie · 2015-11-21T04:47:21Z

@jdm No worries! As someone coming into the project with limited understanding of how all the parts go together, it's actually been pretty useful! Extend away 😄

jdm · 2015-11-21T12:27:22Z

@bors-servo: r+
Hooray!

bors-servo · 2015-11-21T12:27:25Z

📌 Commit 80721a6 has been approved by jdm

bors-servo · 2015-11-21T12:52:39Z

⌛ Testing commit 80721a6 with merge 9d2da0a...


        Auto merge of #8601 - mfeckie:feature/manifest-diff-ci-check, r=jdm

Check if MANIFEST.json changes as per #8587 Adds a script which checks if either of the MANIFEST.json files change in `test/wpt` in response to #8587 - Addresses currently incorrect MANIFEST.json - Adds checking script PR for saltfs to add to CI checks to follow (servo/saltfs#163) https://reviewable.io/reviews/servo/servo/8601  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8601)

bors-servo · 2015-11-21T13:17:39Z

💔 Test failed - linux-rel

jdm · 2015-11-21T14:19:06Z

@bors-servo: retry

#8614

bors-servo · 2015-11-21T14:43:11Z

⌛ Testing commit 80721a6 with merge f2fe117...


        Auto merge of #8601 - mfeckie:feature/manifest-diff-ci-check, r=jdm

Check if MANIFEST.json changes as per #8587 Adds a script which checks if either of the MANIFEST.json files change in `test/wpt` in response to #8587 - Addresses currently incorrect MANIFEST.json - Adds checking script PR for saltfs to add to CI checks to follow (servo/saltfs#163) https://reviewable.io/reviews/servo/servo/8601  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8601)

bors-servo · 2015-11-21T15:12:10Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt


        Auto merge of #163 - mfeckie:feature/manifest-diff-ci-check, r=jdm

Check if manifest file(s) changed Should be merged with servo/servo#8601 Basically uses the script added in that PR to ensure that MANIFEST.json is not changed  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/163)

mfeckie added 3 commits Nov 19, 2015

Commit autogenerated manifest for baseline position

7e5c45d

Add script to check if MANIFEST.json files change

cef839e

Add script to check if MANIFEST.json is changes in wpt

Loading status checks…

3f34ee3

highfive added the S-awaiting-review label Nov 19, 2015

mfeckie mentioned this pull request Nov 19, 2015

Check if manifest file(s) changed servo/saltfs#163

Merged

mfeckie changed the title ~~[WIP] Check if MANIFEST.json changes as per #8587~~ Check if MANIFEST.json changes as per #8587 Nov 19, 2015

Manishearth reviewed Nov 19, 2015
View changes

etc/ci/manifest_changed.sh Outdated

@@ -0,0 +1,4 @@

#!/bin/bash

diff=$(git diff -- tests/wpt/**/MANIFEST.json)

This comment has been minimized.

Sign in to view

Copy link

Manishearth Nov 19, 2015
Member

why the double *?

Run manifest-update without running tests prior to checking diff

Loading status checks…

9c5e9dc

Only use single * and ignore mach output

Loading status checks…

a024c1c

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 20, 2015

Also run checks for test-css MANIFEST.json

Loading status checks…

7fafc6c

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 20, 2015

Add to manifest checks to .travis.yml

Loading status checks…

80721a6

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 21, 2015

jdm self-assigned this Nov 21, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 21, 2015

highfive added S-awaiting-merge and removed S-tests-failed labels Nov 21, 2015

eefriedman mentioned this pull request Nov 27, 2015

Tidy check to make sure manifest items are in order #8269

Closed

wafflespeanut mentioned this pull request Nov 27, 2015

Check that regenerating the test manifest does not change it #8587

Closed

servo / servo

Check if MANIFEST.json changes as per #8587 #8601

Check if MANIFEST.json changes as per #8587 #8601

mfeckie commented Nov 19, 2015

highfive commented Nov 19, 2015

This comment has been minimized.

jdm commented Nov 19, 2015

mfeckie commented Nov 19, 2015

jdm commented Nov 19, 2015

mfeckie commented Nov 19, 2015

jdm commented Nov 20, 2015

mfeckie commented Nov 20, 2015

jdm commented Nov 20, 2015

eefriedman commented Nov 20, 2015

jgraham commented Nov 20, 2015

mfeckie commented Nov 20, 2015

mfeckie commented Nov 20, 2015

jdm commented Nov 20, 2015

mfeckie commented Nov 20, 2015

jdm commented Nov 21, 2015

jdm commented Nov 21, 2015

mfeckie commented Nov 21, 2015

jdm commented Nov 21, 2015

bors-servo commented Nov 21, 2015

bors-servo commented Nov 21, 2015

bors-servo commented Nov 21, 2015

jdm commented Nov 21, 2015

bors-servo commented Nov 21, 2015

bors-servo commented Nov 21, 2015

		@@ -0,0 +1,4 @@
		#!/bin/bash
		diff=$(git diff -- tests/wpt/**/MANIFEST.json)

servo / servo

Join GitHub today

Check if MANIFEST.json changes as per #8587 #8601

Check if MANIFEST.json changes as per #8587 #8601

Conversation

mfeckie commented Nov 19, 2015

highfive commented Nov 19, 2015

This comment has been minimized.

Manishearth Nov 19, 2015

jdm commented Nov 19, 2015

mfeckie commented Nov 19, 2015

jdm commented Nov 19, 2015

mfeckie commented Nov 19, 2015

jdm commented Nov 20, 2015

mfeckie commented Nov 20, 2015

jdm commented Nov 20, 2015

eefriedman commented Nov 20, 2015

jgraham commented Nov 20, 2015

mfeckie commented Nov 20, 2015

mfeckie commented Nov 20, 2015

jdm commented Nov 20, 2015

mfeckie commented Nov 20, 2015

jdm commented Nov 21, 2015

jdm commented Nov 21, 2015

mfeckie commented Nov 21, 2015

jdm commented Nov 21, 2015

bors-servo commented Nov 21, 2015

bors-servo commented Nov 21, 2015

bors-servo commented Nov 21, 2015

jdm commented Nov 21, 2015

bors-servo commented Nov 21, 2015

bors-servo commented Nov 21, 2015