Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken unit tests #16655

Merged
merged 1 commit into from May 3, 2017
Merged

Fix broken unit tests #16655

merged 1 commit into from May 3, 2017

Conversation

@jdm
Copy link
Member

jdm commented Apr 29, 2017

These are tests that only get run on TravisCI, apparently, so they were broken by be0139f and 32c624e without anybody noticing.


This change is Reviewable

@highfive
Copy link

highfive commented Apr 29, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/restyle_hints.rs
  • @emilio: components/style/restyle_hints.rs
@highfive
Copy link

highfive commented Apr 29, 2017

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@jdm jdm mentioned this pull request Apr 29, 2017
3 of 5 tasks complete
@@ -694,6 +694,7 @@ impl DependencySet {
#[cfg(all(test, feature = "servo"))]
fn smoke_restyle_hints() {
use cssparser::Parser;
use selectors::parser::ComplexSelector;

This comment has been minimized.

@emilio

emilio Apr 29, 2017

Member

The signature of note_selector changed, so this is probably not sufficient. :(

@SimonSapin
Copy link
Member

SimonSapin commented Apr 29, 2017

Could you also make ./mach test-unit run these test? So that we don’t regress them again. I think this involves either:

  • In python/servo/testing_commands.py:
    • adding selectors to this set:

          if not packages:
              packages = set(os.listdir(path.join(self.context.topdir, "tests", "unit"))) - set(['.DS_Store'])
    • and adding an exception in this

              for crate in packages:
                  args += ["-p", "%s_tests" % crate]
  • Or moving the tests to a new selectors_tests crate in tests/unit/selectors.
@bors-servo
Copy link
Contributor

bors-servo commented Apr 30, 2017

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

@bholley
Copy link
Contributor

bholley commented Apr 30, 2017

I fixed the selectors tests in #16659

@jdm jdm force-pushed the jdm-patch-1 branch from b64d098 to 177ac6c May 1, 2017
@jdm jdm removed the S-needs-rebase label May 1, 2017
@jdm
Copy link
Member Author

jdm commented May 1, 2017

I've made test-unit run the tests in components/selectors and components/style, and test-unit -p style also runs it.

@SimonSapin
Copy link
Member

SimonSapin commented May 1, 2017

and components/style

Is there any unit tests there? If so we should move them to tests/unit/style instead.

The point of having separate crates for unit tests is to reduce compile times by not compiling large crates like style with rustc --test. (This is less of a concern with selectors which is not as large, and might move back out of the servo/servo repo at some point.)

@jdm
Copy link
Member Author

jdm commented May 1, 2017

@emilio Why did a0c2bdf add a smoke test inside components/style?

@emilio
Copy link
Member

emilio commented May 1, 2017

I believe the idea was not exposing all the stuff in restyle_hints.rs, but I guess given we've already given up on that for the rest of the crate, it doesn't make a lot of sense, so I'll move it to tests/unit/style.

@jdm jdm added S-fails-tidy and removed S-awaiting-review labels May 2, 2017
@jdm
Copy link
Member Author

jdm commented May 2, 2017

$ ./mach test-unit -p selectors

error: package id specification `selectors_tests` matched no packages

The command "./mach test-unit -p selectors" exited with 101.

56.57s$ ./mach cargo test -p style

   Compiling style v0.0.1 (file:///home/travis/build/servo/servo/components/style)

    Finished dev [unoptimized + debuginfo] target(s) in 54.81 secs

     Running /home/travis/build/servo/servo/target/debug/deps/style-b8435dc72cbcc33f

running 1 test

test restyle_hints::smoke_restyle_hints ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured
@jdm jdm mentioned this pull request May 2, 2017
3 of 5 tasks complete
@jdm jdm force-pushed the jdm-patch-1 branch from 177ac6c to 63776ef May 3, 2017
@jdm
Copy link
Member Author

jdm commented May 3, 2017

I moved the unit test out of component/style and into tests/unit/style.

@cbrewster
Copy link
Member

cbrewster commented May 3, 2017

From Travis:

$ ./mach test-unit -p selectors
error: package id specification `selectors_tests` matched no packages
The command "./mach test-unit -p selectors" exited with 101.
@jdm
Copy link
Member Author

jdm commented May 3, 2017

Travis used an old revision for some reason. That error is impossible with the current code.

@jdm jdm force-pushed the jdm-patch-1 branch from 63776ef to fbe0eef May 3, 2017
@highfive highfive removed the S-tests-failed label May 3, 2017
@emilio
Copy link
Member

emilio commented May 3, 2017

@bors-servo r+

Thanks for this :)

@bors-servo
Copy link
Contributor

bors-servo commented May 3, 2017

📌 Commit fbe0eef has been approved by emilio

@highfive highfive assigned emilio and unassigned cbrewster May 3, 2017
@jdm
Copy link
Member Author

jdm commented May 3, 2017

@bors-servo: r-
I want to figure out what Appveyor does not like.

@jdm jdm force-pushed the jdm-patch-1 branch from fbe0eef to f3f9e28 May 3, 2017
@jdm
Copy link
Member Author

jdm commented May 3, 2017

@bors-servo: r=emilio

@bors-servo
Copy link
Contributor

bors-servo commented May 3, 2017

📌 Commit f3f9e28 has been approved by emilio

@bors-servo
Copy link
Contributor

bors-servo commented May 3, 2017

Testing commit f3f9e28 with merge 3905b5a...

bors-servo added a commit that referenced this pull request May 3, 2017
Fix broken unit tests

These are tests that only get run on TravisCI, apparently, so they were broken by be0139f and 32c624e without anybody noticing.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16655)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 3, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: emilio
Pushing 3905b5a to master...

@bors-servo bors-servo merged commit f3f9e28 into master May 3, 2017
3 of 4 checks passed
3 of 4 checks passed
dependency-ci Checking Dependencies
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@SimonSapin SimonSapin deleted the jdm-patch-1 branch May 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.