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

[csp] Wait for all tests to be defined #15631

Merged
merged 1 commit into from
Jun 18, 2019

Conversation

jugglinmike
Copy link
Contributor

Configure the test harness to delay completion until all
asynchronously-defined tests are available.

Configure the test harness to delay completion until all
asynchronously-defined tests are available.
@jugglinmike
Copy link
Contributor Author

Thanks for the review, @andypaicu! Would you mind merging this?

@jugglinmike
Copy link
Contributor Author

@andypaicu seems like this may have slipped off your radar. Would you mind merging it?

foolip added a commit that referenced this pull request Jun 17, 2019
@foolip
Copy link
Member

foolip commented Jun 18, 2019

@jugglinmike, if approved please do merge yourself, it reduces round-trips and would have avoided the possibility of me trying to fix the same bug.

@jugglinmike
Copy link
Contributor Author

@foolip I interpret an approval that isn't immediately followed by merging to mean that further review is expected. If there is substantive feedback or merge conflicts, it's more clear that the ball's in my court. That's not the case here, though. This isn't the first time I've seen this in WPT, so I may be misunderstanding something. How do you interpret this situation?

@foolip
Copy link
Member

foolip commented Jun 18, 2019

@jugglinmike this is a matter of convention and not written down anywhere, but it's very common for people to approve with no comment without merging. I see @Ms2ger do it, and I do it. IMHO it's a good idea to do so if you're not sure if the author might have hoped for someone else to review. Approving with nits and trusting the author to fix those and then merge is also common, typically as "LGTM % nits".

Basically if merging is technically possible it's OK. Reviewers will learn to not approve if that's not what they mean :)

@jugglinmike
Copy link
Contributor Author

Got it. I'll miss having the approval memorialized in the git history, but I'm probably overthinking things. Thanks for the explanation!

@jugglinmike jugglinmike merged commit 159c7c2 into web-platform-tests:master Jun 18, 2019
@foolip
Copy link
Member

foolip commented Jun 18, 2019

I love a good git history, but for this project I don't have high hopes... We have loads of automatic exports with commit messages that are nonsensical taken out of the original context. And people squash-merge PRs without cleaning up the commit messages, making them just a bullet list of somewhat random phrases. Often the useful explanation is in PRs and not in the commit messages.

The wrong committer won't stand out much against this background :D

marcoscaceres pushed a commit that referenced this pull request Jul 23, 2019
Configure the test harness to delay completion until all
asynchronously-defined tests are available.
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.

5 participants