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

Test StorageEvent constructor and initStorageEvent() #13368

Merged
merged 2 commits into from Oct 5, 2018

Conversation

Projects
None yet
5 participants
@foolip
Contributor

foolip commented Oct 4, 2018

Follows whatwg/html#4063.

@foolip foolip referenced this pull request Oct 4, 2018

Merged

Add initStorageEvent() #4063

foolip added a commit to whatwg/html that referenced this pull request Oct 4, 2018

@foolip

This comment has been minimized.

Show comment
Hide comment
@foolip

foolip Oct 4, 2018

Contributor

A little report on failures in various browsers:

  • Chrome+Safari have no required arguments, and key isn't nullable.
  • Edge has 8 (!) required arguments, arguments key, oldValue and newValue aren't nullable, and storageArea is weird because it can take the value undefined in the last test.
  • Firefox: url is nullable, otherwise everything passes

The "initStorageEvent with 8 sensible arguments" test passes on all, suggesting that the code path that matters most is actually pretty fine, interop-wise. (Unsurprising.)

Contributor

foolip commented Oct 4, 2018

A little report on failures in various browsers:

  • Chrome+Safari have no required arguments, and key isn't nullable.
  • Edge has 8 (!) required arguments, arguments key, oldValue and newValue aren't nullable, and storageArea is weird because it can take the value undefined in the last test.
  • Firefox: url is nullable, otherwise everything passes

The "initStorageEvent with 8 sensible arguments" test passes on all, suggesting that the code path that matters most is actually pretty fine, interop-wise. (Unsurprising.)

@annevk

annevk approved these changes Oct 5, 2018

initstorageevent.html could have been .any.js, but this will do.

Show outdated Hide outdated webstorage/event_constructor.html

foolip added some commits Oct 4, 2018

Consolidate/simplify StorageEvent constructor tests
The use of `async_test` was already unnecessary as `dispatchEvent`
synchronously invokes listeners, and use of `dispatcEvent` itself is
not necessary to test the constructor, so simplify to just `test`s.

@foolip foolip changed the title from Test initStorageEvent() to Test StorageEvent constructor and initStorageEvent() Oct 5, 2018

@@ -1,32 +1,81 @@
<!DOCTYPE HTML>
<html>
<head>
<title>WebStorage Test: StorageEvent - init value</title>
<title>WebStorage Test: StorageEvent - constructor</title>

This comment has been minimized.

@foolip

foolip Oct 5, 2018

Contributor

I've run these tests manually. They pass 5/5 in Chrome, Firefox in Safari. In Edge on real hardware and on BrowserStack however, the test seems to crash Edge, saying "This page is having a problem loading". @thejohnjansen FYI, there's probably a real bug here.

@foolip

foolip Oct 5, 2018

Contributor

I've run these tests manually. They pass 5/5 in Chrome, Firefox in Safari. In Edge on real hardware and on BrowserStack however, the test seems to crash Edge, saying "This page is having a problem loading". @thejohnjansen FYI, there's probably a real bug here.

This comment has been minimized.

@foolip

foolip Oct 5, 2018

Contributor

I've narrowed it down to new StorageEvent('storage', { url: null }). I'll file a bug.

@foolip

foolip Oct 5, 2018

Contributor

I've narrowed it down to new StorageEvent('storage', { url: null }). I'll file a bug.

@inexorabletash

looks great, thanks!

foolip added a commit to whatwg/html that referenced this pull request Oct 5, 2018

@foolip

This comment has been minimized.

Show comment
Hide comment
@foolip

foolip Oct 5, 2018

Contributor

I've merged the spec change with the intention of merging this right after, but then saw that Travis was failing. Looks like a network flake, so I've restarted.

Contributor

foolip commented Oct 5, 2018

I've merged the spec change with the intention of merging this right after, but then saw that Travis was failing. Looks like a network flake, so I've restarted.

@foolip foolip merged commit bd5f340 into master Oct 5, 2018

2 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@foolip foolip deleted the foolip/initStorageEvent branch Oct 5, 2018

assert_equals(event.key, null, 'event.key');
assert_equals(event.oldValue, null, 'event.oldValue');
assert_equals(event.newValue, null, 'event.newValue');
assert_equals(event.url, 'undefined', 'event.url');

This comment has been minimized.

@Zirro

Zirro Oct 6, 2018

Member

@foolip I'm implementing initStorageEvent() in jsdom and got an error on this line. Should this be the empty string rather than 'undefined', given the default value for url?

@Zirro

Zirro Oct 6, 2018

Member

@foolip I'm implementing initStorageEvent() in jsdom and got an error on this line. Should this be the empty string rather than 'undefined', given the default value for url?

This comment has been minimized.

@foolip

foolip Oct 6, 2018

Contributor

Yes, this was a mistake. Only the non-optional argument (type) will be the string 'undefined'. Sending fix.

@foolip

foolip Oct 6, 2018

Contributor

Yes, this was a mistake. Only the non-optional argument (type) will be the string 'undefined'. Sending fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment