Add --all option to `mach test-tidy`, with compatibility with --faster #11267

mitchhentges · 2016-05-19T13:32:09Z

Thank you for contributing to Servo! Please replace each [ ] by [X] when the step is complete, and replace __ with appropriate data:

./mach build -d does not report any errors
./mach test-tidy --faster does not report any errors
These changes partially fix Make test-tidy --faster be the default behaviour and add --all parameter #11217

Either:

There are tests for these changes OR
These changes do not require tests because it just affects mach script parameters

Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.

This change is

highfive · 2016-05-19T13:32:14Z

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

highfive · 2016-05-19T13:32:15Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/servo/testing_commands.py, python/tidy/servo_tidy/tidy.py

ghost · 2016-05-19T13:37:23Z

python/servo/testing_commands.py

    @CommandArgument('--self-test', default=False, action="store_true",
                     help="Run unit tests for tidy")
-    def test_tidy(self, faster, no_progress, self_test):
+    def test_tidy(self, all, no_progress, self_test):


We shouldn't use all, since it's a built-in method in python.

How do I make @CommandArgument map from --all to a different variable, like all_files? It doesn't appear to map by parameter index:

@Command('test-tidy', description='Run the source code tidiness check on changed files', category='testing') @CommandArgument('--all', default=False, action="store_true", help="Check all files, and run the WPT lint in tidy, " "even if unchanged") @CommandArgument('--no-progress', default=False, action="store_true", help="Don't show progress for tidy") @CommandArgument('--self-test', default=False, action="store_true", help="Run unit tests for tidy") def test_tidy(self, all_files, no_progress, self_test): if self_test: return test_tidy.do_tests() else: return tidy.scan(all_files, not no_progress)

Produces the output:

TypeError: test_tidy() got an unexpected keyword argument 'all'

I'm not sure whether we can do that with mach. I believe the binding is brought into scope using the string we supply to the decorator. So, we should probably have something like --all-checks

Figured it out! The dest option 😄

That's great news! Thanks :)

ghost · 2016-05-19T13:39:43Z

This doesn't change the current behavior, since only the names of bindings have been changed. We should invert the bool values so that tidy always checks only the changed files.

mitchhentges · 2016-05-19T13:45:26Z

Though default is unchanged, it is properly inverting the behaviour. If just ./mach test-tidy is run, then all is false, and only changed files are checked.
Meanwhile, if ./mach test-tidy --all is run, then all is true, and all files are linted.

This is the inverse of the current behaviour, and what I believe is expected :)

cactipus@SCRATCH-LAPTOP ~/d/servo (test-tidy-sanic-by-default) [1]> 
./mach test-tidy
Checking files for tidiness...
  Progress: 100% (2/2)
tidy reported no errors.
cactipus@SCRATCH-LAPTOP ~/d/servo (test-tidy-sanic-by-default)> 
./mach test-tidy --all
Checking files for tidiness...
  Progress: 0% (84/8488)^Cmach interrupted by signal or user action. Stopping.

emilio · 2016-05-19T13:54:29Z

I think the CI scripts should be updated to use --all

highfive · 2016-05-19T14:00:21Z

New code was committed to pull request.

mitchhentges · 2016-05-19T14:01:19Z

.travis.yml

      script:
-         - ./mach test-tidy --self-test
-         - ./mach test-tidy --no-progress
+         - ./mach test-tidy --no-progress --self-test


I'm not sure if this is a good idea.
I saw that buildbot_steps.yml has --no-progress with --self-test, and I'm assuming that they're supposed to be the same? Perhaps they just fell out-of-sync, with regard to parameters?

aneeshusa · 2016-05-19T14:15:47Z

@mitchhentges The actual CI scripts live in the servo/saltfs repo, in the buildbot/master/files/config/steps.yml file. We're in the process of switching over to read the scripts from this repo, but in the meantime you'll need to make a PR against servo/saltfs.

This also means that in order to keep full test coverage, this PR will need to be split in two - first we need to land a PR that adds the --all option while keeping --faster (without changing the default), then land a PR in saltfs to add the --all option to the CI scripts, then land a followup PR here to switch the default over (and get rid of --faster).

mitchhentges · 2016-05-19T14:18:26Z

Sure, no problem, I'll make this test only add --all, and remove the other changes. I'll wait to change this until I've got an idea of how to specify --all without using the variable all

ghost · 2016-05-19T16:29:21Z

python/tidy/servo_tidy/tidy.py

+def scan(all=False, progress=True):
    # standard checks
-    files_to_check = filter_files('.', faster, progress)
+    only_changed_files = not all


Sorry, I missed this in my first look 😅

highfive · 2016-05-19T17:35:01Z

New code was committed to pull request.

mitchhentges · 2016-05-19T17:40:23Z

And here's what it looks like, with different combos of parameters:

./mach test-tidy
Checking files for tidiness...
  Progress: 1% (99/8488)^Cmach interrupted by signal or user action. Stopping.
cactipus@SCRATCH-LAPTOP ~/d/servo (test-tidy-sanic-by-default) [1]> 
./mach test-tidy --all
Checking files for tidiness...
  Progress: 0% (80/8488)^Cmach interrupted by signal or user action. Stopping.
cactipus@SCRATCH-LAPTOP ~/d/servo (test-tidy-sanic-by-default) [1]> 
./mach test-tidy --faster
Checking files for tidiness...
  Progress: 100% (2/2)
tidy reported no errors.
cactipus@SCRATCH-LAPTOP ~/d/servo (test-tidy-sanic-by-default)> 
./mach test-tidy --faster --all
Cannot tidy --all while also being --faster

highfive · 2016-05-19T17:41:12Z

New code was committed to pull request.

mitchhentges · 2016-05-19T17:42:18Z

python/tidy/servo_tidy/tidy.py


-def filter_files(start_dir, faster, progress):
-    file_iter = get_file_list(start_dir, faster, ignored_dirs)
+def filter_files(start_dir, only_changed_files, progress):


I changed this file a little bit, because I think that only_changed_files is a little more descriptive than faster

ghost · 2016-05-19T17:42:45Z

LGTM. r? @aneeshusa

Ms2ger · 2016-05-20T09:49:48Z

Maybe --full is better than --all, if we don't like all?

mitchhentges · 2016-05-20T09:52:52Z

I think that --all was only an issue, because the mapped variable clashed with Python's all function.
Fortunately, with CommandParameter's dest argument, the flag could be mapped to something else, like all_files.

I think it's OK now, with --all?

Ms2ger · 2016-05-20T09:55:59Z

wfm

aneeshusa · 2016-05-24T19:54:29Z

This looks good to me, just needs a squash. A few comments:

I found it strange that we don't use all_files in testing_commands.py, but the comment you wrote was very helpful for my understanding.
Switching to only_changed_files is a nice cleanup, and I don't think there are any back-compat issues for servo-tidy as a separate Python lib.

aneeshusa · 2016-05-24T22:50:42Z

@bors-servo r+

You can make the follow-up PR while we wait for this one to land. (Queue's a bit long today: http://build.servo.org/homu/queue/all)

bors-servo · 2016-05-24T22:50:43Z

📌 Commit 29da462 has been approved by aneeshusa

mitchhentges · 2016-05-24T22:55:59Z

Already done :)
servo/saltfs#377

aneeshusa · 2016-05-24T22:59:44Z

Right, I meant a followup for the servo repo where we actually change the default over. I checked the saltfs PR and it LGTM, just waiting for this to land.

Thanks again for the PR.

bors-servo · 2016-05-24T23:00:53Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Make /dom/nodes/CharacterData-surrogates.html not panic. #10796

bors-servo · 2016-05-24T23:00:54Z

📌 Commit 29da462 has been approved by aneeshusa

bors-servo · 2016-05-24T23:54:13Z

⌛ Testing commit 29da462 with merge b6e06bd...

…eshusa Add --all option to `mach test-tidy`, with compatibility with --faster Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy --faster` does not report any errors - [X] These changes partially fix #11217 Either: - [ ] There are tests for these changes OR - [X] These changes do not require tests because it just affects `mach` script parameters Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11267)

bors-servo · 2016-05-25T00:18:36Z

💔 Test failed - mac-rel-wpt

cbrewster · 2016-05-25T00:23:31Z

@bors-servo retry

Intermittents with FAILED TO establish the default connection to the WindowServer #11396

bors-servo · 2016-05-25T00:53:04Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, windows are reusable. Rebuilding only mac-rel-wpt...

larsbergstrom · 2016-05-25T01:16:43Z

@bors-servo retry

servo-mac1 getting OSX 10.11 installed

bors-servo · 2016-05-25T01:16:45Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, windows are reusable. Rebuilding only mac-rel-wpt...

bors-servo · 2016-05-25T01:56:09Z

💔 Test failed - mac-rel-wpt

highfive · 2016-05-25T01:56:11Z

  ▶ CRASH [expected PASS] /_mozilla/css/style_is_in_doc.html
  │ 
  │ Shutting down the Constellation after generating an output file or exit flag specified
  │ thread &#39;FileManager&#39; panicked at &#39;called `Result::unwrap()` on an `Err` value: IoError(Error { repr: Custom(Custom { kind: Other, error: StringError(&#34;Unknown Mach error: 46&#34;) }) })&#39;, ../src/libcore/result.rs:785
  │ stack backtrace:
  │    1:        0x1098faf28 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
  │    2:        0x109901475 - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
  │    3:        0x10990108e - std::panicking::default_hook::hc2c969e7453d080c
  │    4:        0x109674da2 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::ha2bd86c312dc8d7a
  │    5:        0x1098e8782 - std::panicking::rust_panic_with_hook::hfe203e3083c2b544
  │    6:        0x109901a36 - std::panicking::begin_panic::h4889569716505182
  │    7:        0x1098ea078 - std::panicking::begin_panic_fmt::h484cd47786497f03
  │    8:        0x10990168f - rust_begin_unwind
  │    9:        0x109928e80 - core::panicking::panic_fmt::h257ceb0aa351d801
  │   10:        0x1080c2727 - core::result::unwrap_failed::h1d94443d858a91df
  │   11:        0x1080c18e3 - std::panicking::try::call::ha6de14882521e730
  │   12:        0x109904bfb - __rust_try
  │   13:        0x109904b95 - __rust_maybe_catch_panic
  │   14:        0x1080c21ff - _&lt;F as std..boxed..FnBox&lt;A&gt;&gt;::call_box::h82ed0593b19c5256
  │   15:        0x1099003d8 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  │   16:     0x7fff95828059 - _pthread_body
  │   17:     0x7fff95827fd6 - _pthread_start
  │ Shutting down the Constellation after generating an output file or exit flag specified
  │ thread &#39;ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(0) }&#39; panicked at &#39;index out of bounds: the len is 0 but the index is 0&#39;, ../src/libcollections/vec.rs:1167
  │ stack backtrace:
  │    1:        0x1094e3f28 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
  │    2:        0x1094ea475 - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
  │    3:        0x1094ea08e - std::panicking::default_hook::hc2c969e7453d080c
  │    4:        0x10925dda2 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::ha2bd86c312dc8d7a
  │    5:        0x1094d1782 - std::panicking::rust_panic_with_hook::hfe203e3083c2b544
  │    6:        0x1094eaa36 - std::panicking::begin_panic::h4889569716505182
  │    7:        0x1094d3078 - std::panicking::begin_panic_fmt::h484cd47786497f03
  │    8:        0x1094ea68f - rust_begin_unwind
  │    9:        0x109511e80 - core::panicking::panic_fmt::h257ceb0aa351d801
  │   10:        0x109512000 - core::panicking::panic_bounds_check::h3956fdcea61ff421
  │   11:        0x10830da4e - script::dom::browsingcontext::BrowsingContext::active_document::h226f24d08ec44427
  │   12:        0x10831268a - script::dom::browsingcontext::BrowsingContext::active_window::h2ba128d21d9d19cf
  │   13:        0x10888b88c - _&lt;script_thread..ScriptMemoryFailsafe&lt;&#39;a&gt; as core..ops..Drop&gt;::drop::hc9b03780a30d666b
  │   14:        0x10888e69c - std::panicking::try::call::h8d69dc69dac5d183
  │   15:        0x1094edbfb - __rust_try
  │   16:        0x1094edb95 - __rust_maybe_catch_panic
  │   17:        0x10888ec94 - _&lt;F as std..boxed..FnBox&lt;A&gt;&gt;::call_box::h2e5eb7eb1a952230
  │   18:        0x1094e93d8 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  │   19:     0x7fff95828059 - _pthread_body
  │   20:     0x7fff95827fd6 - _pthread_start
  └ thread panicked while panicking. aborting.

cbrewster · 2016-05-25T02:02:14Z

@bors-servo retry

Intermittent crash in /_mozilla/css/style_is_in_doc.html #11400

bors-servo · 2016-05-25T02:35:35Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, windows are reusable. Rebuilding only mac-rel-wpt...

bors-servo · 2016-05-25T03:14:47Z

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

Use --all flag when running `./mach test-tidy` [`./mach test-tidy` should be `--faster` by default](servo/servo#11217). However, the [CI server should still check everything using the `--all` flag](servo/servo#11267 (comment)). This PR changes the CI server to use the `--all` flag, but shouldn't be merged until Servo's [`11267`](servo/servo#11267) gets merged, because `11267` adds `--all`  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/377)

…husa Remove --faster flag from test-tidy, go fast by default. Fixes 11217 Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11217 (github issue number if applicable). Either: - [ ] There are tests for these changes OR - [X] These changes do not require tests because they just change command-line options, which aren't tested Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. This finishes #11267  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11409)

highfive assigned pcwalton May 19, 2016

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

ghost reviewed May 19, 2016
View reviewed changes

ghost assigned ghost and unassigned pcwalton May 19, 2016

mitchhentges reviewed May 19, 2016
View reviewed changes

ghost reviewed May 19, 2016
View reviewed changes

mitchhentges force-pushed the test-tidy-sanic-by-default branch from 03d94e7 to 316f9cb Compare May 19, 2016 17:34

mitchhentges changed the title ~~Only test_tidy on changed files by default~~ Add --all option to mach test-tidy, with compatibility with --faster May 19, 2016

mitchhentges reviewed May 19, 2016
View reviewed changes

highfive assigned aneeshusa and unassigned ghost May 19, 2016

mitchhentges mentioned this pull request May 19, 2016

Use --all flag when running ./mach test-tidy servo/saltfs#377

Merged

ghost mentioned this pull request May 19, 2016

Should we check the diffs whenever new code is committed? servo/highfive#127

Open

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 May 24, 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 May 25, 2016

cbrewster mentioned this pull request May 25, 2016

Intermittents with FAILED TO establish the default connection to the WindowServer #11396

Closed

highfive added the S-tests-failed The changes caused existing tests to fail. label May 25, 2016

bors-servo merged commit 29da462 into servo:master May 25, 2016

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

mitchhentges mentioned this pull request May 25, 2016

Remove --faster flag from test-tidy, go fast by default. Fixes 11217 #11409

Merged

5 tasks

mitchhentges deleted the test-tidy-sanic-by-default branch May 25, 2016 08:26

Uh oh!

Add --all option to mach test-tidy, with compatibility with --faster #11267

Add --all option to mach test-tidy, with compatibility with --faster #11267

Uh oh!

Conversation

mitchhentges commented May 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented May 19, 2016

Uh oh!

highfive commented May 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented May 19, 2016

Uh oh!

mitchhentges commented May 19, 2016

Uh oh!

emilio commented May 19, 2016

Uh oh!

highfive commented May 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aneeshusa commented May 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mitchhentges commented May 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

highfive commented May 19, 2016

Uh oh!

mitchhentges commented May 19, 2016

Uh oh!

highfive commented May 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented May 19, 2016

Uh oh!

Ms2ger commented May 20, 2016

Uh oh!

mitchhentges commented May 20, 2016

Uh oh!

Ms2ger commented May 20, 2016

Uh oh!

aneeshusa commented May 24, 2016

Uh oh!

aneeshusa commented May 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors-servo commented May 24, 2016

Uh oh!

mitchhentges commented May 24, 2016

Uh oh!

aneeshusa commented May 24, 2016

Uh oh!

bors-servo commented May 24, 2016

Uh oh!

bors-servo commented May 24, 2016

Uh oh!

bors-servo commented May 24, 2016

Uh oh!

bors-servo commented May 25, 2016

Uh oh!

cbrewster commented May 25, 2016

Uh oh!

bors-servo commented May 25, 2016

Uh oh!

larsbergstrom commented May 25, 2016

Uh oh!

Add --all option to `mach test-tidy`, with compatibility with --faster #11267

Add --all option to `mach test-tidy`, with compatibility with --faster #11267

mitchhentges commented May 19, 2016 •

edited

Loading

aneeshusa commented May 19, 2016 •

edited

Loading

aneeshusa commented May 24, 2016 •

edited

Loading