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

Tests for ResizeObserver #6878

Merged
merged 4 commits into from
Oct 2, 2017
Merged

Tests for ResizeObserver #6878

merged 4 commits into from
Oct 2, 2017

Conversation

atotic
Copy link
Contributor

@atotic atotic commented Aug 14, 2017

ResizeObserver is a new DOM API that we are planning to ship in Chrome.

https://github.com/WICG/ResizeObserver
Chrome tracking bug at: https://crbug.com/612962

These tests will help other platforms implement a spec-compliant, inter operable API.

An intern at Firefox has worked on implementing it last year, and we shared these tests.

@wpt-pr-bot
Copy link
Collaborator

There are no owners for this pull request. Please reach out on W3C's irc server (irc.w3.org, port 6665) on channel #testing (web client) to get help with this. Thank you!

@ghost
Copy link

ghost commented Aug 14, 2017

Build ERRORED

Started: 2017-08-14 22:19:13
Finished: 2017-08-14 22:41:00

View more information about this build on:

@atotic
Copy link
Contributor Author

atotic commented Aug 15, 2017

I've looked at the travis error. It looks like a timeout....

@ghost
Copy link

ghost commented Sep 5, 2017

Build PASSED

Started: 2017-10-02 17:06:32
Finished: 2017-10-02 17:13:53

Failing Jobs

  • safari:10.0
  • MicrosoftEdge:14.14393

View more information about this build on:

@foolip
Copy link
Member

foolip commented Sep 30, 2017

@atotic, have these already been reviewed once in Chromium? If so it doesn't need another full review and I can just merge this. Note that you can also just move tests to LayoutTests/external/wpt in Chromium and a PR just like this will be automatically created and merged, if that's more convenient for you.

@foolip
Copy link
Member

foolip commented Sep 30, 2017

@atotic, one thing that you can't do from within the Chromium tree is to create OWNERS files of the correct shape. Can you add one here and add people from as many different engines as possible?

@atotic
Copy link
Contributor Author

atotic commented Oct 2, 2017

These tests are already in the Chromium tree, and will be deleted once this pull request goes through.

I will create the OWNERS file. The only person from different engines that has collaborated on these tests is mozilla's @dholbert. Is there anyone else you suggest I add?

@foolip
Copy link
Member

foolip commented Oct 2, 2017

Nope, just add the people you know of, I don't know anything about resize observer :)

Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

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

I have skimmed the tests and am approving this to merge once Travis is happy. I'll leave a few comments for consideration.


var onErrorCalled = false;

window.onerror = err => {
Copy link
Member

Choose a reason for hiding this comment

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

err will be bound to an event, and in any case it's not used, so _ or () would be more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do.

guard = async_test('guard');
}, "ResizeObserver implemented")

test0()
Copy link
Member

Choose a reason for hiding this comment

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

If you want the tests to be run in sequence, have you tried promise_test? They are run one after another by default. Is it that it's pointless to run the following tests if the first does not pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the pointer. I was unaware that promise_tests are sequential.

My existing framework works well with RO callback pattern. It is not immediately obvious to me how to map chained RO callbacks into promise_test pattern. I'll think about it....


let guard;
test(_ => {
assert_own_property(window, "ResizeObserver");
Copy link
Member

Choose a reason for hiding this comment

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

This guard is a bit curious, why is it needed? new ResizeTestHelper will throw an exception anyway because it tried to do new ResizeOberserver. Tests shouldn't pass if the feature isn't implemented, but the explicit guard for it is a bit unusual.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Without the guard, tests used to timeout because ResizeTestHelper would fail in constructor before async tests were created. I can fix this.

@foolip foolip merged commit 022f6d9 into web-platform-tests:master Oct 2, 2017
@foolip
Copy link
Member

foolip commented Oct 2, 2017

@atotic, once this is imported in Chromium, it may be easier to iterate there. But wherever you prefer, here you can get review from @dholbert :)

jakearchibald pushed a commit to jakearchibald/web-platform-tests that referenced this pull request Nov 16, 2017
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.

3 participants