improve test-tidy

[tigercosmos]

check wpt manifest when run tidy

If CI has already run test-tidy, and then no need to run etc/ci/manifest_changed.sh

---

This change is 

[CYBAI]

How about putting this checkbox under There are tests for these changes?

For example,

<!-- Either: -->
- [ ] There are tests for these changes
  - [ ] `./etc/ci/manifest_changed.sh` does not report any errors

<!-- OR -->
- [ ] These changes do not require tests because _____

[tigercosmos]

Put Either is good, but we need to mention that ./etc/ci/manifest_changed.sh is only for WPT

[jdm]

What if we run the manifest_changed script as part of test-tidy if there are any files changed in test/wpt instead?

[CYBAI]

@jdm oh! wow! Sounds a good solution for me!

[tigercosmos]

Sounds good.

[tigercosmos]

r? jdm

[CYBAI]

I found the first diff in .travis.yml, we will run ./mach test-tidy --no-progress --self-test in Travis.
With the diff in this line, we won't check manifest_changed under --self-test.
So, if we removed bash etc/ci/manifest_changed.sh in it, then we won't check manifest changed.

Besides, it might be better to check manifest_changed when there's any WPT related file changes?

[tigercosmos]

--self-test is for unit test. we don't need to update manifest for unit test.
I think the best way to detect WPT change is using manifest_changed, and this doesn't take much time, so this might be an Okay solution.

[jdm]

Have you tested what happens now when you run ./mach test-tidy when the manifest does need updating? What does the output look like? I'm concerned that we suppress any non-zero return value from run_update right now.

[tigercosmos]

@jdm you are right... error doesn't return non-zoro.

tigercosmos@ubuntu:~/servo$ ./mach test-tidy
 0:11.98 INFO Diffing old and new manifests /home/tigercosmos/servo/tests/wpt/mozilla/meta/MANIFEST.json
 0:12.47 INFO Diffing old and new manifests /home/tigercosmos/servo/tests/wpt/metadata/MANIFEST.json
 0:14.73 /home/tigercosmos/servo/tests/wpt/metadata/MANIFEST.json  0  error  core-aam/erer.html in source but not in manifest.  (wpt-manifest)
 0:14.99 ERROR Manifest /home/tigercosmos/servo/tests/wpt/metadata/MANIFEST.json is outdated, use |./mach update-manifest| to fix.
Checking the config file...
Checking the wpt manifest file...
Checking directories for correct file extensions...
Checking files for tidiness...
  Progress: 100% (3/3)
tidy reported no errors.

[tigercosmos]

I have added system exit for this.

tigercosmos@ubuntu:~/servo$ ./mach test-tidy
 0:09.36 INFO Diffing old and new manifests /home/tigercosmos/servo/tests/wpt/mozilla/meta/MANIFEST.json
 0:09.71 INFO Diffing old and new manifests /home/tigercosmos/servo/tests/wpt/metadata/MANIFEST.json
 0:11.32 /home/tigercosmos/servo/tests/wpt/metadata/MANIFEST.json  0  error  core-aam/erer.html in source but not in manifest.  (wpt-manifest)
 0:11.49 ERROR Manifest /home/tigercosmos/servo/tests/wpt/metadata/MANIFEST.json is outdated, use |./mach update-manifest| to fix.

[CYBAI]

In my personal opinion, I'll prefer to keep run_update returning its status and check the status to exit the program here.

if self_test:
    return test_tidy.do_tests()
else:
    if run_update(self.context.topdir, check_clean=True):
        sys.exit(1) # Maybe we can `return 1` instead?
    return tidy.scan(not all_files, not no_progress, stylo=stylo) 

# OR

if self_test:
    return test_tidy.do_tests()
else:
    return run_update(self.context.topdir, check_clean=True) and tidy.scan(not all_files, not no_progress, stylo=stylo) 

[tigercosmos]

sounds good to me

[jdm]

The commit message no longer accurately describes the changes :)

[jdm]

What if we do this instead:

manifest_dirty = run_update(self.context.topdir, check_clean=True)
tidy_failed = tidy.scan(not all_files, not no_progress, stylo=stylo)
return tidy_failed or manifest_dirty

[jdm]

Let's remove manifest_changed.sh since no code uses it any more.

[tigercosmos]

OK

[jdm]

@tigercosmos could you change the commit message to read "Check WPT manifest when running test-tidy."?

[tigercosmos]

@jdm done

[jdm]

@bors-servo r+

[bors-servo]

📌 Commit 80c41b4 has been approved by jdm

[bors-servo]

⌛ Testing commit 80c41b4 with merge b7d44e0...

[bors-servo]

💔 Test failed - mac-rel-css2

[jdm]

@bors-servo retry

[bors-servo]

⌛ Testing commit 80c41b4 with merge 01d5765...

[bors-servo]

💔 Test failed - linux-rel-wpt

[jdm]

@bors-servo retry

• #21067

[bors-servo]

⌛ Testing commit 80c41b4 with merge 7f74513...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 7f74513 to master...