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

Added Unbind tests #174

Merged
merged 2 commits into from Feb 28, 2016
Merged

Conversation

mschultz4
Copy link
Contributor

In response to issue #166, I added a couple of more tests for Unbind. Please let me know if you see potential improvements.

@LeaVerou
Copy link
Owner

Thanks so much!!
We don't yet implement multiple namespaces per event though, so those tests should fail. Wanna comment them out until we do?

@mschultz4
Copy link
Contributor Author

Certainly. Which tests should I comment out exactly? I am not sure I understand which will fail. Thanks

@LeaVerou
Copy link
Owner

Just convert events that use namespaces like click.namespace1.foo to use one namespace, e.g. either click.namespace1 or click.foo.
However, did these pass when you run them? If so, there must be some bug in them, since the functionality they're testing is not supported!

@mschultz4
Copy link
Contributor Author

Oh ok, I see. That test was actually written by @jxc876 within request #169 and merged already. The reason the test is passing is because the listeners are attached to the first namespace and not the second level. I am not sure why the second namespace was originally included, but it adds nothing to the test and is obviously confusing so I removed it.

When I attach the listeners to the second namespace and run the test, they do indeed fail as expected.

@LeaVerou
Copy link
Owner

Cool! Please make these changes to this PR and it's good to go!

@mschultz4
Copy link
Contributor Author

Awesome! I already included the changes in my commit.

LeaVerou added a commit that referenced this pull request Feb 28, 2016
@LeaVerou LeaVerou merged commit e629898 into LeaVerou:gh-pages Feb 28, 2016
@LeaVerou
Copy link
Owner

Oh, I hadn't noticed. Merged now, thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants