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

Run CSS Multicolumn tests automatically #13599

Merged
merged 1 commit into from
Oct 6, 2016
Merged

Run CSS Multicolumn tests automatically #13599

merged 1 commit into from
Oct 6, 2016

Conversation

slester
Copy link

@slester slester commented Oct 5, 2016

Implements the CSS multicolumn tests as outlined in issue #13587.


  • There are tests for these changes OR
  • These changes do not require tests because they implement tests for other features.

This change is Reviewable

@highfive
Copy link

highfive commented Oct 5, 2016

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

@highfive
Copy link

highfive commented Oct 5, 2016

Heads up! This PR modifies the following files:

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 5, 2016
@jdm
Copy link
Member

jdm commented Oct 5, 2016

@bors-servo: r+
Thank you very much!

@bors-servo
Copy link
Contributor

📌 Commit 37d6725 has been approved by jdm

@highfive highfive assigned jdm and unassigned metajack Oct 5, 2016
@highfive 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 Oct 5, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit 37d6725 with merge 8d4ff7e...

bors-servo pushed a commit that referenced this pull request Oct 5, 2016
Run CSS Multicolumn tests automatically

<!-- Please describe your changes on the following line: -->
Implements the CSS multicolumn tests as outlined in issue #13587.

---
<!-- 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 #13587

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because they implement tests for other features.

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

<!-- 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/13599)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-css

@highfive 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 Oct 5, 2016
@jdm
Copy link
Member

jdm commented Oct 5, 2016

Looks like only one change is required - make the expected outcome of multicol-br-inside-avoidcolumn-001.htm (insidemulticol-br-inside-avoidcolumn-001.htm.ini) platform specific:

expected:
  if os == "linux": TIMEOUT
  CRASH

@highfive 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 Oct 5, 2016
@slester
Copy link
Author

slester commented Oct 5, 2016

All right, these changes have been made! Sorry about that.

@jdm
Copy link
Member

jdm commented Oct 5, 2016

There's no reason to apologize; inconsistent test expectations are a problem with our test harness, not something that you're responsible for :)
@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit 2f6393a has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit 2f6393a with merge 0ff6874...

@highfive 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 Oct 5, 2016
bors-servo pushed a commit that referenced this pull request Oct 5, 2016
Run CSS Multicolumn tests automatically

<!-- Please describe your changes on the following line: -->
Implements the CSS multicolumn tests as outlined in issue #13587.

---
<!-- 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 #13587

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because they implement tests for other features.

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

<!-- 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/13599)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-css

@highfive 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 Oct 5, 2016
@jgraham
Copy link
Contributor

jgraham commented Oct 5, 2016

The manifest files are whitespace-sensitive so you can't have

if os == "linux": TIMEOUT
  CRASH

but must instead have the same whitespace:

if os == "linux": TIMEOUT
CRASH

@jdm
Copy link
Member

jdm commented Oct 5, 2016

I'm surprised that the code that checks if the manifests change on TravisCI didn't report any error about that...

@jgraham
Copy link
Contributor

jgraham commented Oct 5, 2016

Different manifests… it could of course but at the moment it doesn't because we don't read expectation metadata for tests we won't run.

@highfive 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 Oct 5, 2016
@slester
Copy link
Author

slester commented Oct 5, 2016

Third time's a charm, hopefully.

@jdm
Copy link
Member

jdm commented Oct 5, 2016

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit ec8ade4 has been approved by jdm

@highfive highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Oct 5, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit ec8ade4 with merge 6bd8986...

@highfive highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 5, 2016
bors-servo pushed a commit that referenced this pull request Oct 5, 2016
Run CSS Multicolumn tests automatically

<!-- Please describe your changes on the following line: -->
Implements the CSS multicolumn tests as outlined in issue #13587.

---
<!-- 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 #13587

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because they implement tests for other features.

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

<!-- 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/13599)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

@bors-servo bors-servo merged commit ec8ade4 into servo:master Oct 6, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run CSS Multicolumn tests automatically
6 participants