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 inheritance.sub.html WPT to work on Servo #31534

Merged
merged 1 commit into from Mar 8, 2024

Conversation

azharcodeit
Copy link
Contributor

@azharcodeit azharcodeit commented Mar 6, 2024

Fix inheritance.sub.html WPT to work on Servo


  • There are tests for these changes

@servo-wpt-sync
Copy link
Collaborator

🤖 Opened new upstream WPT pull request (web-platform-tests/wpt#44954) with upstreamable changes.

Copy link
Contributor

@Loirooriol Loirooriol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, but while you are at it you should purge the old entries from the expectation files, like

rm tests/wpt/meta/css/css-backgrounds/inheritance.sub.html.ini
./mach test-wpt -r --log-raw /tmp/servo.log --headless tests/wpt/tests/css/css-backgrounds/inheritance.sub.html
./mach update-wpt /tmp/servo.log

rm tests/wpt/meta-legacy-layout/css/css-backgrounds/inheritance.sub.html.ini
./mach test-wpt --legacy-layout -r --log-raw /tmp/servo.log --headless tests/wpt/tests/css/css-backgrounds/inheritance.sub.html
./mach update-wpt --legacy-layout /tmp/servo.log

(Typically you don't need to remove to update the expectations, but in this case it's for purging the obsolete entries)

@@ -31,7 +31,7 @@
</style>
<script>
const transparentColor = 'rgba(0, 0, 0, 0)'; // https://www.w3.org/TR/css-color-3/#transparent
const mediumWidth = getComputedStyle(document.getElementById('reference')).columnRuleWidth; // e.g. 3px
const mediumWidth = '3px';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be good to link to https://www.w3.org/TR/css-backgrounds-3/#valdef-line-width-medium

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Loirooriol, addressed fixes in updated PR, please have a look, thank you!

@Loirooriol
Copy link
Contributor

Oh, and when you post a pull request, you should mark the checkboxes in the template, like

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #31518 (GitHub issue number if applicable)
- [X] These changes do not require tests because this is just fixing a test

(arguably fixing a test could also count as "There are tests for these changes", pick whatever you think makes more sense)

@servo-wpt-sync
Copy link
Collaborator

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#44954).

@Loirooriol
Copy link
Contributor

You added an empty patch, possibly by mistake and you wanted to push another one?

@azharcodeit
Copy link
Contributor Author

You added an empty patch, possibly by mistake and you wanted to push another one?

Yes, working on it, pulled main branch so must have been automatically updated

@servo-wpt-sync
Copy link
Collaborator

✍ Updated existing upstream WPT pull request (web-platform-tests/wpt#44954) title and body.

2 similar comments
@servo-wpt-sync
Copy link
Collaborator

✍ Updated existing upstream WPT pull request (web-platform-tests/wpt#44954) title and body.

@servo-wpt-sync
Copy link
Collaborator

✍ Updated existing upstream WPT pull request (web-platform-tests/wpt#44954) title and body.

@servo-wpt-sync
Copy link
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@servo-wpt-sync
Copy link
Collaborator

✍ Updated existing upstream WPT pull request (web-platform-tests/wpt#44954) title and body.

Copy link
Contributor

@Loirooriol Loirooriol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good!

BTW, it's noteworthy that since this is a WPT change, it has been exported to that repo: web-platform-tests/wpt#44954
There you can go to the "Checks" tab to ensure that this isn't breaking on other browsers. Effectively, chrome, firefox and safari were passing 64/64 before the change, and still 64/64 after the change, so everything is good.

@azharcodeit
Copy link
Contributor Author

Thanks, looks good!

BTW, it's noteworthy that since this is a WPT change, it has been exported to that repo: web-platform-tests/wpt#44954 There you can go to the "Checks" tab to ensure that this isn't breaking on other browsers. Effectively, chrome, firefox and safari were passing 64/64 before the change, and still 64/64 after the change, so everything is good.

Great! I will check it, thank you

auto-merge was automatically disabled March 7, 2024 10:06

Merge queue setting changed

@Loirooriol
Copy link
Contributor

Ah, #31453 landed first, so now there is a conflict in tests/wpt/meta/css/css-backgrounds/inheritance.sub.html.ini
Can you rebase the patch to the current tip?

auto-merge was automatically disabled March 7, 2024 14:46

Head branch was pushed to by a user without write access

@servo-wpt-sync
Copy link
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@servo-wpt-sync
Copy link
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@sagudev
Copy link
Member

sagudev commented Mar 7, 2024

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

web-platform-tests/wpt#44954 does not include comment with spec link, so it's really out of sync.

@Loirooriol
Copy link
Contributor

It's strange, because initially the bot could export the changes without problems, and the test hasn't been changed upstream: https://github.com/web-platform-tests/wpt/commits/master/css/css-backgrounds/inheritance.sub.html

I will investigate tomorrow.

@servo-wpt-sync
Copy link
Collaborator

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#44954).

@Loirooriol
Copy link
Contributor

I squashed the commits together and now it works.

I suspect the problem was that d5908c9 has no WPT modification, and

"commit", "--message", commit["message"], "--author", commit["author"]
isn't using --allow-empty.

@Loirooriol Loirooriol added this pull request to the merge queue Mar 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 8, 2024
@Loirooriol Loirooriol added this pull request to the merge queue Mar 8, 2024
@mrobinson
Copy link
Member

Looks like the failure here was the typical "hdiutil: create failed - Resource busy" that we see.

Merged via the queue into servo:main with commit a5a0e1c Mar 8, 2024
9 checks passed
@azharcodeit azharcodeit deleted the fix/servo-31518 branch March 27, 2024 22:48
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.

Fix inheritance.sub.html WPT to work on Servo
5 participants