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

[css-values] Add tests for basic min() / max() support #19801

Closed
wants to merge 2 commits into from

Conversation

foolip
Copy link
Member

@foolip foolip commented Oct 21, 2019

Resubmitted from #10377. Original description:

https://drafts.csswg.org/css-values-4/#functional-notations

Tests cover the following:

  • attr() in max()
  • calc() in max()
  • max() with 20 arguments
  • min() with unitless 0

This is my first wpt test. Comments are very welcome. Thanks!

https://drafts.csswg.org/css-values-4/#functional-notations

Tests cover the following:

- attr() in max()
- calc() in max()
- max() with 20 arguments
- min() with unitless 0
@foolip
Copy link
Member Author

foolip commented Oct 21, 2019

@xfq can you paste the commands that led to the mishap in #10377? Maybe I can spot where it went wrong.

Here's what I did to recreate a branch for this PR.

git fetch origin refs/pull/10377/head
git log FETCH_HEAD

FETCH_HEAD (4c76917) was a merge commit, and it looked like the same commits were on both parents of the merge commit, eb9f5f1 and 5da5b1d. So I created a new branch for each.

git checkout -b xfq-1 FETCH_HEAD^1
git checkout -b xfq-2 FETCH_HEAD^2

xfq-1 was already just two commits on top of origin/master, so I rebased xfq-2 on origin/master too: git rebase origin/master.

Then I checked if it was the same as xfq-1, and it was: git diff xfq-1 xfq-2.

So I pushed the xfq-1 branch to the repo:

git checkout -b xfq/css-values-min-max xfq-1
git push -u origin xfq/css-values-min-max

And that's what I created this PR from.

I also filed web-platform-tests/wpt-pr-bot#81 about something to help rebase PRs that could avoid mishaps like this.

@foolip
Copy link
Member Author

foolip commented Oct 21, 2019

Note that I first tried to just push directly to the branch in your fork, but I didn't have permission. That's because the maintainer_can_modify was false in https://api.github.com/repos/web-platform-tests/wpt/pulls/10377. I think it should be set to true by default for new PRs.

@foolip
Copy link
Member Author

foolip commented Oct 21, 2019

@xfq you can see the results for this in https://wpt.fyi/results/?label=pr_head&max-count=1&pr=19801, are they what you expected?

Since there might be additional changes required here, I'd suggest submitting a new PR from the same branch or a new one. I'll then close this PR.

@xfq
Copy link
Contributor

xfq commented Oct 22, 2019

@xfq can you paste the commands that led to the mishap in #10377? Maybe I can spot where it went wrong.

I am not 100% sure, but in my memory, I executed:

git rebase master
git merge master
git push origin xfq/css-values-min-max

(origin is my fork of the WPT repo.)

@xfq
Copy link
Contributor

xfq commented Oct 22, 2019

@xfq you can see the results for this in https://wpt.fyi/results/?label=pr_head&max-count=1&pr=19801, are they what you expected?

I haven't looked at the (rendered) test results yet, but since only Safari supports min() / max(), and in this case only Safari passed some of the tests, I didn't notice any issue.

Since there might be additional changes required here, I'd suggest submitting a new PR from the same branch or a new one. I'll then close this PR.

I'll try to do that. Thank you very much, @foolip!

@xfq
Copy link
Contributor

xfq commented Oct 22, 2019

Resubmitted in #19819.

@foolip
Copy link
Member Author

foolip commented Oct 22, 2019

Closing in favor of #19819.

@xfq regarding the sequence of git commands in #19801 (comment), assuming your branch was originally pointing to 5da5b1d, I don't think that could have produced the result with the two commits on both sides of the merge commit, as rebasing on any commit and then trying to merge it it would result in the merge doing nothing with a "Already up to date" message.

What might have happened is that you first rebased locally and then merged in the remote branch which was still pointing to the previous state.

I guess we'll just leave this a mystery, but if you get into a similar state again and would like help understanding what went wrong, I'd be happy to help. I've messed up rebases and merges enough times myself :)

@foolip foolip closed this Oct 22, 2019
@foolip foolip deleted the xfq/css-values-min-max branch October 22, 2019 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants