fix #14040: QUnit.reset() replaced by splitting up into individual tests

[lexi-sh]

All unit tests that were changed are passing. JShint done without errors. Pull request fixes this ticket: http://bugs.jquery.com/ticket/14040

[markelog]

/cc @Krinkle, @jzaefferer

[timmywil]

Thank you!

Please sign our CLA.

[markelog]

@timmywil he already signed it.

[timmywil]

@markelog Ah, I searched for "Chris Jones" rather than "Christopher".

[Krinkle]

@cjqed 🌟 Thanks!

[lexi-sh]

My pleasure. Is there anything else I need to do?

[dmethvin]

Nope, this should be good. We're aiming for a beta3 next week and it should land before that. Thanks a lot for tackling this, it's one of those tasks that has no glory so it's often ignored. I'll add a star as well: 🌟 😺

[markelog]

@cjqed there is also some calls of QUnit.reset in testrunner.js, they are also mentioned in the ticket description, would you like to handle them as well?

[lexi-sh]

@markelog Sure, I saw it originally and wasn't exactly sure how to handle it but upon re-reading some of the code in there I see what needs to be done. I'll do that and commit to this branch.

[lexi-sh]

@markelog Is there anything else I need to do? Thanks!

[dmethvin]

@cjqed I'm getting an error when I build this and run unit tests, if you're available can you take a look? Probably something simple.

[lexi-sh]

@dmethvin Sorry! Didn't notice that one. Try now?

[markelog]

@cjqed Do not add empty.js, use either innerHTML prop or create jQuery.config.fixture as jQuery object beforehand.

Otherwise it could generate some weird problems in the future, since our tests, unfortunately, depend on each other.

[markelog]

@cjqed It looks to me that instead of removing QUnit.reset we now have our own private QUnit.reset that uses undocumented features and has a vague logic :-(.

[lexi-sh]

@markelog I understand. The problem I'm running into is that sizzle is trying to access a relative path empty.js at ~/src/sizzle/test/index.html here:

<script id="script-src" src="data/empty.js"></script>

This is fine becuase ~/src/sizzle/test/data/empty.js exists. However, when running sizzle tests from the QUnit GUI, we get a file not found error (causing the odd issue with "unexpected token <") at ~/test/data/empty.js (Not surprising, looking for the relative path). I figured adding the empty file would be better than changing the src to an absolute path.

It looks to me that instead of removing QUnit.reset we now have our own private QUnit.reset that uses undocumented features and has a vague logic :-(.

Yeah, I wasn't too happy with that. Let me go back to the drawing board for the reset() function.

[markelog]

trying to access a relative path empty.js at

Yeah, jQuery#html reprocess this script and tries to load it through our ajax module, by using innerHTML for example, you can avoid that.

Let me go back to the drawing board for the reset() function.

You could remove most of it logic, by making tests more atomic, like code for ajax and globals cleanup, but our integration tests for sizzle do not let you remove fixture chaos, unless you would make a PR for Sizzle repo as well.

An even then, we would still require requireFixture helper to stay on, but i guess it's another story, btw could you rebase?

[lexi-sh]

I was thinking of looking at the logic for requireFixture and reset and at least trying to get around using the undocumented interface QUnit.config.fixture. I'm not really well-versed with QUnit itself though, by the way you make it sound it would be larger issue?

Since retooling the fixture chaos sounds necessary to fixing this and is a larger issue, I could git revert back to the commit without touching the testrunner.js so that at least QUnit.reset() isn't used in the actual tests and tests become a bit more atomic and rebase. I can then work on cleaning up sizzle testing for a separate pull request.

Is this a good idea, or would you want everything in the same pull request?

[markelog]

I could git revert

You don't have to revert, just remove empty.js, replace

jQuery("#qunit-fixture").html(QUnit.config.fixture);

with

jQuery( "#qunit-fixture" )[ 0 ].innerHTML = QUnit.config.fixture;

Rebase and make sure everything works in 1.x-master, if there some merge issue it would more applicable to make a separate PR for 1.x-master too, then, i think, it would ready as is, for now.

I can then work on cleaning up sizzle testing for a separate pull request.

I would suggest you to start with removal of Globals methods, since they unneeded in master branch, then ajax atomicity, then Sizzle. Maybe even, some of those blocks are not meant to be removed, but we should try to bring down "hacks" to the minimum.

But in any case, it would be great if you could help us out.

[timmywil]

just remove empty.js

This is automatically copied over when we run grunt bower. Can we do this without removing empty.js?

[markelog]

@timmywil no it's not. By

remove empty.js

i meant the one on the test/data/empty.js path, not src/sizzle/test/data/empty.js

[markelog]

btw you can remove empty.js from sizzle repo too, but i do not mean you have to, personally, i would prefer it to stay.

[timmywil]

i meant the one on the test/data/empty.js path

gotcha, thanks for clarifying.

[lexi-sh]

@markelog Hey, sorry for the question but I use hg at work and am still pretty wet behind the ears on git and rebasing. I made the changes and committed it to my 14040-remove-qunit-reset branch. Do I git checkout master, git rebase 14040-remove-qunit-reset and then push remote? I think that would squash the commit history on this branch into my local master branch, but this pull request is from the 14040 branch so it wouldn't do anything in this PR would it?

Again, sorry!

[markelog]

Do the backup copy of your branch first.

Do I git checkout master

Yes, then pull from upstream, if haven't add an upstream, do it like that – git remote add upstream git@github.com:jquery/jquery.git,

then pull from it – git pull upstream master, then checkout 14040-remove-qunit-reset
and make a rebase – git rebase master, now your commits should be on top.

Then push. That should do it.

[lexi-sh]

@markelog Did I do that correctly?

My last commit seems to fail the travis build, but it looks like it's failing on npm install, possibly having to do with 1bb6951?

[lexi-sh]

@markelog Okay, did some more reading and realized I did not do that correctly. Instead of trying to fix the branch and have it be incredibly messy, would you mind if I created a new pull request from a new branch that's fresh from master?

[dmethvin]

You can git push --force on this existing branch and it will wipe away your sins but keep the discussion, which is useful.

[lexi-sh]

@dmethvin Thanks for the tip! That looks much better, and the travis build passed. Sorry for the issues!

[dmethvin]

@cjqed Thanks! 🌟 I landed this in master. I also tried cherry-picking it into the 1.x-master branch but it has enough variations there to make it a non-trivial exercise. Would you like to give that a try as well, and open a new PR for that? You're getting quite a workout for your first contribution!

[lexi-sh]

@dmethvin Sure, I'll do the same for 1.x-master. Thanks! Happy to help 👍