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 {transform,perspective}-origin accepts (and ignores) anything for… #15613
Conversation
Can you add the test case in the linked issue? |
Of course. Can you say, in what folder it should be, and how to correct test this behavior? |
Probably add a new file You can see other file for how property parsing can be tested. |
You can run the tests with |
@KiChjang tests were added |
☔ The latest upstream changes (presumably #15732) made this pull request unmergeable. Please resolve the merge conflicts. |
rebased now |
tests/unit/style/parsing/effects.rs
Outdated
|
||
let mut parser = Parser::new("1px 2px rubbish"); | ||
let parsed = longhands::parse_origin(&context, &mut parser); | ||
assert_eq!(parsed.is_ok(), true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert!(parsed.is_ok())
should work just fine here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, not in this case. Exhaustion is checked in another part of the code, not in longhand parsing. This parser will return Ok
but fail at is_exhausted
.
And there is assert_parser_exhausted
macro for this kind of tests. Could you use this one please? You can see the usage in tests/unit/style/parsing/border.rs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exhaustion check is actually below this line, but if there's a macro for checking it, then we should use that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry I thought you were saying that we can delete second assertion. I didn't look closely to your example :)
I think there should be a test checking parsing |
@KiChjang please review now. macro |
@bors-servo r+ |
📌 Commit 89f96c0 has been approved by |
fix {transform,perspective}-origin accepts (and ignores) anything for… … their second part of value <!-- 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 #15487. <!-- Either: --> - [X] There are tests for these changes OR <!-- 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/15613) <!-- Reviewable:end -->
☀️ 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-gnu-dev, windows-msvc-dev |
… their second part of value
./mach build -d
does not report any errors./mach test-tidy
does not report any errorsThis change is