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

Test the delegate interface. #3077

Closed
wants to merge 1 commit into from
Closed

Test the delegate interface. #3077

wants to merge 1 commit into from

Conversation

braddunbar
Copy link
Collaborator

Since Backbone doesn't implement attaching event handlers or triggering DOM events, there's no reason for us to test it. Instead, we can test the interface for delegate and delegateEvents to ensure the correct arguments are passed at the correct time.

This means that code from Backbone.$ isn't actually under test and therefore doesn't need to be shimmed for cases like #3073.

@wyuenho
Copy link
Contributor

wyuenho commented Mar 19, 2014

This has pretty wide-ranging implications. I like that the tests are more unit-y this way, but I'm not convinced that you can avoid testing behaviors entirely. Methods don't just take some parameter and return a value, they do things and create side-effects too. We really need to make sure those side-effects are there.

@wyuenho
Copy link
Contributor

wyuenho commented Mar 19, 2014

Is there anyway to test for behaviors and side effects without shimming?

@braddunbar
Copy link
Collaborator Author

I don't quite follow. What side effects do you mean?

The only thing Backbone is concerned with is calling #delegate and $el.on with the correct arguments. We rely completely on the DOM implementation (jQuery or otherwise) to call us back when a specific event occurs.

@akre54
Copy link
Collaborator

akre54 commented Mar 19, 2014

Methods don't just take some parameter and return a value, they do things and create side-effects too.

Agreed. How else do you propose we test for behavior in adapters, @braddunbar?

@braddunbar
Copy link
Collaborator Author

How else do you propose we test for behavior in adapters, @braddunbar?

I assume an adapter would have tests of it's own as they don't belong here. If Backbone is to be agnostic as to the DOM implementation used it's tests should be too.

@akre54
Copy link
Collaborator

akre54 commented Mar 19, 2014

But Backbone's view test covers common features to all implementations (like like ensuring we can pass a string as el, or delegating click listeners). Forcing the view implementations to keep up with a moving target is a recipe for disaster. Just look at how small the changes are to support Backbone.$ in #3073.

@braddunbar
Copy link
Collaborator Author

But Backbone's view test covers common features to all implementations (like like ensuring we can pass a string as el, or delegating click listeners).

The goal of extracting Backbone.View's DOM operations into separate methods was to ensure that they could be replaced without otherwise changing or reimplementing behavior. Consequently, common features don't need to be replaced. That was the point, right?

Forcing the view implementations to keep up with a moving target is a recipe for disaster.

I don't follow. What's moving exactly? Backbone's interface hasn't changed and neither has it's default method of event delegation.

Updated to include several more tests.

@jashkenas
Copy link
Owner

While I appreciate the sentiment, having a sanity test that Backbone View events are continuing to work properly with jQuery seems like a wise idea. It's not just about use testing the contract — it's also about being warned early if anything funny happens in future versions of jQuery (as has happened before).

I think we should be testing the real thing, not just that the arguments we pass are the arguments we pass.

@jashkenas jashkenas closed this Mar 26, 2014
@braddunbar braddunbar deleted the delegate branch March 26, 2014 17:29
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.

4 participants