Skip to content

/ servo

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

Remove whitespace when serializing operators in attribute selectors #17203

Closed
wants to merge 4 commits into from
Closed

Remove whitespace when serializing operators in attribute selectors #17203

wants to merge 4 commits into from
+23 −23

Conversation

@achiwhane
Copy link
Contributor

achiwhane commented Jun 7, 2017 •
edited

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #17181.
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable
@highfive
Copy link

highfive commented Jun 7, 2017

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

jdm commented Jun 7, 2017

From ./mach test-unit -p style:

---- parser::tests::test_parsing stdout ----

	thread 'parser::tests::test_parsing' panicked at 'assertion failed: `(left == right)` (left: `"[attr|=\"foo\"]"`, right: `"[attr |= \"foo\"]"`)', /home/travis/build/servo/servo/components/selectors/parser.rs:1717
@Manishearth
Copy link
Member

Manishearth commented Jun 9, 2017

You'll need to update the tests in components/selectors/parser.rs to be ok with spaces too.
@Manishearth Manishearth mentioned this pull request Jun 9, 2017
Closed
4 of 4 tasks complete
@Manishearth
Copy link
Member

Manishearth commented Jun 9, 2017

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Jun 9, 2017

⌛ Trying commit ff0efae with merge 60d2bd5...
bors-servo added a commit that referenced this pull request Jun 9, 2017
@bors-servo
Auto merge of #17203 - achiwhane:master, r=<try>
Loading status checks…
60d2bd5 
Remove whitespace when serializing operators in attribute selectors

<!-- Please describe your changes on the following line: -->
---
<!-- 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 #17181.

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

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

bors-servo commented Jun 9, 2017

💔 Test failed - linux-rel-wpt
@Manishearth
Copy link
Member

Manishearth commented Jun 9, 2017

@bors-servo try

hm, infra
@achiwhane
Copy link
Contributor Author

achiwhane commented Jun 9, 2017 •
edited

Hey, sorry for the delay. I'm out of town right now and will get this fixed in the next few days.
@Manishearth
Copy link
Member

Manishearth commented Jun 9, 2017

@jyc had fixes for the tests here, perhaps he can help take this over the finish line?
@highfive highfive removed the S-tests-failed label Jun 9, 2017
@Manishearth
Copy link
Member

Manishearth commented Jun 9, 2017 •
edited

Please rebase your changes instead of merging, git rebase origin/master

(followed by a force-push)
achiwhane added 4 commits Jun 7, 2017
@achiwhane
Remove whitespace when serializing operators in attribute selectors
40c7330
@achiwhane
Remove whitespace from selectors in unit tests.
3e073ce
@achiwhane
Update selector test case
Loading status checks…
84cc105
@achiwhane
Remove trailing whitespace
Loading status checks…
683f0ce
@Manishearth
Copy link
Member

Manishearth commented Jun 13, 2017

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jun 13, 2017

📌 Commit 683f0ce has been approved by Manishearth
@highfive highfive assigned Manishearth and unassigned mbrubeck Jun 13, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Jun 13, 2017

⌛ Testing commit 683f0ce with merge 84b2aed...
bors-servo added a commit that referenced this pull request Jun 13, 2017
@bors-servo
Auto merge of #17203 - achiwhane:master, r=Manishearth
Loading status checks…
84b2aed 
Remove whitespace when serializing operators in attribute selectors

<!-- Please describe your changes on the following line: -->
---
<!-- 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 #17181.

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

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

bors-servo commented Jun 13, 2017

💔 Test failed - linux-rel-wpt
@jdm
Copy link
Member

jdm commented Jun 13, 2017 

  ▶ Unexpected subtest result in /selectors/attribute-selectors/attribute-case/cssom.html:
  └ PASS [expected FAIL] [foo="bar"] /* sanity check */ getting CSSRule#cssText

  ▶ Unexpected subtest result in /selectors/attribute-selectors/attribute-case/cssom.html:
  └ PASS [expected FAIL] [foo="bar"] /* sanity check */ getting CSSRule#cssText in @media

  ▶ Unexpected subtest result in /selectors/attribute-selectors/attribute-case/cssom.html:
  └ PASS [expected FAIL] [foo="bar" i] getting CSSRule#cssText

  ▶ Unexpected subtest result in /selectors/attribute-selectors/attribute-case/cssom.html:
  └ PASS [expected FAIL] [foo="bar" i] getting CSSRule#cssText in @media

  ▶ Unexpected subtest result in /selectors/attribute-selectors/attribute-case/cssom.html:
  └ PASS [expected FAIL] [foo="bar" /**/ i] getting CSSRule#cssText

  ▶ Unexpected subtest result in /selectors/attribute-selectors/attribute-case/cssom.html:
  └ PASS [expected FAIL] [foo="bar" /**/ i] getting CSSRule#cssText in @media

  ▶ Unexpected subtest result in /selectors/attribute-selectors/attribute-case/cssom.html:
  └ PASS [expected FAIL] [foo="bar"/**/i] getting CSSRule#cssText

  ▶ Unexpected subtest result in /selectors/attribute-selectors/attribute-case/cssom.html:
  └ PASS [expected FAIL] [foo="bar"/**/i] getting CSSRule#cssText in @media
@Manishearth
Copy link
Member

Manishearth commented Jun 13, 2017 •
edited by jdm

Please apply the following patch after rebasing on to master:

(Alternatively, use https://github.com/manishearth/servo/tree/pr-17203 or tick the "allow maintainers to push changes" box on this pull request)

diff --git a/tests/wpt/metadata/selectors/attribute-selectors/attribute-case/cssom.html.ini b/tests/wpt/metadata/selectors/attribute-selectors/attribute-case/cssom.html.ini
index 4b2a69d..ed81ef0 100644
--- a/tests/wpt/metadata/selectors/attribute-selectors/attribute-case/cssom.html.ini
+++ b/tests/wpt/metadata/selectors/attribute-selectors/attribute-case/cssom.html.ini
@@ -1,65 +1,41 @@
 [cssom.html]
   type: testharness
-  [[foo="bar"\] /* sanity check */ getting CSSRule#cssText]
-    expected: FAIL
-
   [[foo="bar"\] /* sanity check */ getting CSSStyleRule#selectorText]
     expected: FAIL
 
   [[foo="bar"\] /* sanity check */ setting CSSStyleRule#selectorText]
     expected: FAIL
 
-  [[foo="bar"\] /* sanity check */ getting CSSRule#cssText in @media]
-    expected: FAIL
-
   [[foo="bar"\] /* sanity check */ getting CSSStyleRule#selectorText in @media]
     expected: FAIL
 
   [[foo="bar"\] /* sanity check */ setting CSSStyleRule#selectorText in @media]
     expected: FAIL
 
-  [[foo="bar" i\] getting CSSRule#cssText]
-    expected: FAIL
-
   [[foo="bar" i\] getting CSSStyleRule#selectorText]
     expected: FAIL
 
-  [[foo="bar" i\] getting CSSRule#cssText in @media]
-    expected: FAIL
-
   [[foo="bar" i\] getting CSSStyleRule#selectorText in @media]
     expected: FAIL
 
-  [[foo="bar" /**/ i\] getting CSSRule#cssText]
-    expected: FAIL
-
   [[foo="bar" /**/ i\] getting CSSStyleRule#selectorText]
     expected: FAIL
 
   [[foo="bar" /**/ i\] setting CSSStyleRule#selectorText]
     expected: FAIL
 
-  [[foo="bar" /**/ i\] getting CSSRule#cssText in @media]
-    expected: FAIL
-
   [[foo="bar" /**/ i\] getting CSSStyleRule#selectorText in @media]
     expected: FAIL
 
   [[foo="bar" /**/ i\] setting CSSStyleRule#selectorText in @media]
     expected: FAIL
 
-  [[foo="bar"/**/i\] getting CSSRule#cssText]
-    expected: FAIL
-
   [[foo="bar"/**/i\] getting CSSStyleRule#selectorText]
     expected: FAIL
 
   [[foo="bar"/**/i\] setting CSSStyleRule#selectorText]
     expected: FAIL
 
-  [[foo="bar"/**/i\] getting CSSRule#cssText in @media]
-    expected: FAIL
-
   [[foo="bar"/**/i\] getting CSSStyleRule#selectorText in @media]
     expected: FAIL

 </details>
@jdm
Copy link
Member

jdm commented Jun 20, 2017

@achiwhane Are you planning to address the previous comment?
@bors-servo
Copy link
Contributor

bors-servo commented Jun 21, 2017

⌛ Testing commit 683f0ce with merge b6e774d4662799b4c07f59b6a897a2b4c076acb5...
@bors-servo
Copy link
Contributor

bors-servo commented Jun 21, 2017

💔 Test failed - linux-rel-wpt
@jyc jyc mentioned this pull request Jun 30, 2017
Merged
3 of 5 tasks complete
@jdm
Copy link
Member

jdm commented Jun 30, 2017

@bors-servo: r-
@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

☔ The latest upstream changes (presumably #17822) made this pull request unmergeable. Please resolve the merge conflicts.
@jdm
Copy link
Member

jdm commented Aug 10, 2017

Closing due to inactivity.
@jdm jdm closed this Aug 10, 2017
@jdm jdm mentioned this pull request Aug 10, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Manishearth Manishearth

Labels
S-needs-rebase S-tests-failed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Do not add spaces when serializing operators in attribute selectors
6 participants
@achiwhane @highfive @jdm @Manishearth @bors-servo @mbrubeck
You can’t perform that action at this time.