Skip to content

IndexedDB: test a self-removing 'complete' handler. #15822

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

quasicomputational
Copy link
Contributor

At least one mocking implementation has a bug that this code tickles -
see dumbmatter/fakeIndexedDB#26. Since I'd already reduced it down to
a few lines of code, it might as well become a WPT.

Copy link
Contributor

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test! Turning regression tests for implementation bugs into a shared quality foundation for the platform is great.

};
tx.addEventListener('complete', complete);
const store = tx.objectStore(STORE);
store.get(0).addEventListener("success", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

needs t.step_func

@quasicomputational quasicomputational force-pushed the indexeddb-removelistener branch from e74b542 to 4481db9 Compare March 13, 2019 19:21
@quasicomputational
Copy link
Contributor Author

Thanks for the review! I think I got to all of the suggestions. The one thing I didn't do was to use one of the more framework-y helpers, since this code is testing a buggy event listener implementation, but those helpers add listeners for various things themselves and I'm worried it might wind up not tickling the original bug if it's too far removed from the reproducer.

Copy link
Contributor

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

Few more really minor things. Thanks again!

At least one mocking implementation has a bug that this code tickles -
see dumbmatter/fakeIndexedDB#26. Since I'd already reduced it down to
a few lines of code, it might as well become a WPT.
@quasicomputational quasicomputational force-pushed the indexeddb-removelistener branch from 4481db9 to 4f60f62 Compare March 14, 2019 14:52
@quasicomputational
Copy link
Contributor Author

Ping. I think I've addressed all the comments. I dunno what's up with Taskcluster being red, though.

@Hexcles Hexcles closed this Apr 26, 2019
@Hexcles Hexcles reopened this Apr 26, 2019
@quasicomputational
Copy link
Contributor Author

Ping?

Copy link
Contributor

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

Looks good; sorry about forgetting about this and thanks for pinging.

@marcoscaceres
Copy link
Contributor

Might need a rebase...

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.

7 participants