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

Remove remaining jQuery from View tests #3073

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

akre54
Copy link
Collaborator

@akre54 akre54 commented Mar 18, 2014

Now that #3003 is merged, we need an easy way for alternative View implementations to run against our View conformance test. This pulls jQuery out of the View tests except in for checking view.$el instanceof Backbone.$ (which all jQuery-like apis should support).

@eastridge
Copy link
Contributor

@akre54 should Backbone.Nativeview provide a Backbone.$ to check against?

@akre54
Copy link
Collaborator Author

akre54 commented Mar 19, 2014

Why? It's not present and not needed.

@eastridge
Copy link
Contributor

"which all jQuery-like apis should support" was what made me think of that, but I guess it's not really a jQuery API, but something that makes Backbone not need jQuery

initialize: function() {
this.listenTo(this.model, 'all x', function(){ ok(false); });
this.listenTo(this.collection, 'all x', function(){ ok(false); });
this.listenTo(this.model, 'all x', function(){ ok(false); }, this);
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need the this in the end.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Thanks.

@wyuenho
Copy link
Contributor

wyuenho commented Mar 19, 2014

Mind if I refactor the dispatchEvent shim and fix the custom event test?

@akre54
Copy link
Collaborator Author

akre54 commented Mar 19, 2014

Go for it. Not sure why Travis is throwing a fit. Maybe we have to expose CustomEvent from window?

@wyuenho
Copy link
Contributor

wyuenho commented Mar 19, 2014

Nah, non-standard custom events just need to be created in a special way.

https://developer.mozilla.org/en/docs/Web/API/CustomEvent

@braddunbar
Copy link
Collaborator

Take a look at #3077. I think that sort of thing will make this process much easier.

@akre54
Copy link
Collaborator Author

akre54 commented Mar 19, 2014

@braddunbar reverted View -> Backbone.View changes, and removed the setup method which was only being used to test the constructor. As long as the view implementation keeps a reference to Backbone.View (if needed), it should be fine to reassign to Backbone.View for testing purposes.

@wyuenho
Copy link
Contributor

wyuenho commented Apr 4, 2014

This looks good to me. It turns out there wasn't an easy way to fire a custom event on IE<=8.

Also, in case anyone is wondering why there's a click function, ariya/phantomjs#10795 is why.

@wyuenho
Copy link
Contributor

wyuenho commented Jun 17, 2014

Hey guys, can this be merged or is there something else you think we should do?

@braddunbar
Copy link
Collaborator

I do not think this one should be merged. From @jashkenas in #3077:

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.

Testing via mocked event creation is certainly not the real thing. Further, it somewhat obscures the contract.

@akre54
Copy link
Collaborator Author

akre54 commented Jun 17, 2014

Testing via mocked event creation is certainly not the real thing

How is this different from what we have now with $el.trigger('click')?

Tests aren't the place to catch issues with the latest jQuery. We've faced it before (with PATCH support, e.g.), and there almost always is a reasonable solution. Specs should never test your dependencies.

@braddunbar
Copy link
Collaborator

How is this different from what we have now with $el.trigger('click')?

jQuery#trigger is tested and used in real application code and a one-off implementation in our test suite isn't?

Tests aren't the place to catch issues with the latest jQuery. We've faced it before (with PATCH support, e.g.), and there almost always is a reasonable solution. Specs should never test your dependencies.

I'd prefer contract only tests in this case but using a trusted and tested library like jQuery is certainly better than an untested implementation written just for our tests.

@akre54
Copy link
Collaborator Author

akre54 commented Jun 17, 2014

Totally, and I can appreciate that. But this isn't some far-out custom code that we just pulled out of thin air. jQuery's code itself is more synthetic than what we're doing here since its on/trigger lifecycle lives entirely within the jQuery realm. Triggering a MouseEvent directly is much closer to what we're actually trying to test here (jquery itself uses elem.click() to trigger events bound outside of $.fn.on).

};

function click(element) {
var event;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey @wyuenho, how come we couldn't just use element.click() directly here? It'd obviate this messiness here. Only downside I see is that the element would have to live in the DOM already.

Copy link
Contributor

Choose a reason for hiding this comment

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

ariya/phantomjs#10795

We can and should get rid of the click shim as soon as that phantom bug gets fixed. Either that or we use a different test runner like Karma that actually tests on real browsers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it might be nice to get something like Sauce labs testing in here too since we're concerned about older browsers.

Shame about that bug though.

@akre54
Copy link
Collaborator Author

akre54 commented Feb 17, 2015

@braddunbar want to take one final pass through this and then either merge or close? I'm fine either way.

@wyuenho
Copy link
Contributor

wyuenho commented Apr 12, 2015

@akre54 Has PhantomJS 2 fixed this click event issue?

@jgonggrijp jgonggrijp added this to Fridge in Dusting off Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants