-
Notifications
You must be signed in to change notification settings - Fork 20.5k
Preserve originalEvent property (for compat) #2336
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To all - i believe this is a best way for creating fixtures, not in the actual qunit html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For improved atomicity and encapsulation.
- If this test would become obsolete and you would need to remove it you might forget to remove them too, this happened couple times before.
- If there would be different assertion like
equal( $( "div" ).length, 2 )you would break it - Other tests might start use those fixtures too and then when you would need to change it, for regression testing or such, you might break other tests.
And stuff like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sure, I thought you were saying this was better in all cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely like having the test markup in the test. When I was rewriting events back in 1.7 the big index.html was a nightmare to follow so I tended to do what @markelog did here for new tests. For selector tests I think it's good to have a reasonably complex document to query against though, so I guess it depends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To all - i believe this is a best way for creating fixtures, not in the actual qunit html.
I agree, but we should be creating and injecting content with supportjQuery instead of jQuery (i.e., a fixed version distinct from the code under test).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use of supportjQuery would help you create an isolated env for you, that is cool, but that is a unit-test which means you testing smallest possible action and change should be atomic, so really, you would need to use this entity only when you create an environment through the thing you test - only in if you expect that your changes could affect other parts of library.
And even if your change has effect on assertion output it probably be false negative then false positive which would additionally indicate that something is wrong.
In other words, if you test jQuery.get you could use jQuery#html in your tests, because your changes on jQuery.get shouldn't influence entirely different module (at least in most cases) since it should be atomic and even if it does, then jQuery.get assertions would fail then give right output. But if you need to use jQuery.get to create fixtures for jQuery.get assertions then you should use supportjQuery.
I'm not sure what is right thing to do, best action might be to go from the real-life example, although it seems supportjQuery is the most correct way. But then there is a consistency issue, we are not gonna rewrite 6 thousands tests because of it, do we?
Now, tests with wrong kinds of fixtures is another story, we probably should start rewriting at least some of them, since we need it to use more flexible testing tools like karma.
|
If there is no additional objections, i would like to merge this tomorrow |
Could you apply all the fixes from comments first so that we can look at the final PR? |
|
Sure |
|
Updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for whom -> for which
|
One comment nit but otherwise LGTM 👍 |
|
Landed via 37c3d08 |
Alternative to gh-2300