Limit all test suite selection operations to #qunit-fixture

[gibson042]

For those that are explicitly document-scope, wrap with supportjQuery( $jQueryResults ).filter( "#qunit-fixture *" ) (cf. #2877 (comment) ).

[markelog]

I believe i have fixed that, there might be more, but they are not causing issues at the moment.

See b97c8d3 f1300f1

Note: that should be done for sizzle too.

[gibson042]

Why did you close this? It is a general hygiene ticket that should stay open until we are reasonably confident that there are no "others".

[markelog]

I believe i have addressed your comment mentioned in this ticket description, aside from resolved cases there is no proof that such tests even exist, they might, but it is not determined, so i think we reached "reasonable" criteria, otherwise i don't understand how you want determine if those selectors are there.

Also, i think bad hygiene is also to have tickets that are open forever, without any possibility of closing except for subjective feeling of confidence.

However, if you strongly feel about this, please feel free to re-open.

[gibson042]

i don't understand how you want determine if those selectors are there

By looking through all the unit tests.

i think bad hygiene is also to have tickets that are open forever, without any possibility of closing except for subjective feeling of confidence.

I agree, but closing it now is indistinguishable from "wontfix". The point here is to avoid re-occurrence, and if you're looking for an objective characterization, here you go:

• Someone comments on this issue certifying that they have examined all unit tests and found no selections scoped so broadly that they could match QUnit output, or
• A commit lands on the master branch claiming to correct all such selections

[gibson042]

• Someone comments on this issue certifying that they have examined all unit tests and found no selections scoped so broadly that they could match QUnit output, or
• A commit lands on the master branch claiming to correct all such selections

The first of these should be discounted, because I've already found some offending examples:

• attributes.js#L223
• attributes.js#L261
• attributes.js#L494-L496
• core.js#L769
• core.js#L777
• css.js#L482
• css.js#L570

I'll stop there, but you get the point.

[markelog]

Even though for most of those tests it doesn't matter what element is taken, it should work for any of them, but now i understand your train of thought.

A commit lands on the master branch claiming to correct all such selections

I would propose to do it gradually, since it could be a lot of work to do in action, one commit would be preferable, yeah, but it hard to do, for example i can take attributes module tonight.

I agree, but closing it now is indistinguishable from "wontfix". The point here is to avoid re-occurrence, and if you're looking for an objective characterization

Well, i always comment about such tests and fix a lot of them already, not even counting those aforementioned commits, so i think we no longer write such fragile tests.

[alisianoi]

Hi, I decided to hijack limiting test suit operations to #qunit-fixture in the attributes.js module from @markelog but immediately hit a bump.

So, @gibson042 mentions attributes.js#L223 as one of the candidates for explicit limiting but not necesserily the first one. Then I look at the test above it and see an element with #text1 being used. Is it obvious that this element is in #qunit-fixture (i.e. doesn't need to be explicitely filtered)? It becomes obvious to me only after looking inside the index.html.

[gibson042]

@all3fox This ticket is only about operation safety, so it's appropriate to leave selections like "#text1". Problematic cases will almost always start with element selectors, since they are most likely to inadvertently match content beyond our control (e.g., from QUnit).

[alisianoi]

Ok, I did grep -n "jQuery( " test/unit/attributes.js and then ignored all the selections that use:

• an id
• on-the-fly created markup
• an explicit wrap of an element (like L108 or L121)
• an xml element as context (like L192)

I was left with:

• a L1030 where it is unlikely but possible to select outside #qunit-fixture
• L1166, L1190 and L1239, where "inheritance" is used: jQuery("#qunit-fixture div")
• and lines mentioned by @gibson042 above

My question now is: how do you want these fixed:

• with supportjQuery().filter()
• with inheritance jQuery("#qunit-fixture div")
• or with context selection: jQuery("div", "#qunit-fixture ")?

Personally, I do not understand the supportjQuery thingy defined in here. What does it do?

[gibson042]

Ok, I did grep -n "jQuery( " test/unit/attributes.js and then ignored all the selections that use:

• an id
• on-the-fly created markup
• an explicit wrap of an element (like L108 or L121)
• an xml element as context (like L192)

Explicit jQuery() wraps are not necessarily good. L108 begins a section of code that manipulates attributes on document.body rather than content under #qunit-fixture or dynamically generated, which is bad form. That's not in the purview of this issue, but selections would be.

My question now is: how do you want these fixed:

• with supportjQuery().filter()
• with inheritance jQuery("#qunit-fixture div")
• or with context selection: jQuery("div", "#qunit-fixture ")?

jQuery( "#qunit-fixture …" ) is the most straightforward means. .filter() would be reserved for exceptional cases in which selection broader than #qunit-fixture is required for proper testing, and there shouldn't be any of those in attributes.js.

Personally, I do not understand the supportjQuery thingy defined in here. What does it do?

supportjQuery is a fixed copy of jQuery (and would be better named jQueryStable), independent of the one under test (see test/index.html and test/jquery.js). It should be used whenever a test needs to rely on jQuery functionality that it is not itself being tested, so e.g. manipulation tests could succeed even if the dist/ jQuery had flaws with selection. I doubt we'll ever get to that level of separation, and honestly it's mostly an academic issue because the core functionality of jQuery is so stable that we rarely see problems from relying on the system under test, however distasteful the practice is.

[mgol]

Another offender is the "child and adjacent" selector test. Running the whole test suite in Chrome 48 almost always fails the p > * > * assertion for me.

[gibson042]

Heh, that was actually the first identified offender: #2877 (comment)

[alisianoi]

@mgol I see that the function assert.t( a, b, c ) from test/data/testinit.js is used, with b being the selector. Apart from test/unit/selector.js, this function is used in test/unit/css.js and test/unit/manipulation.js. It looks to me like it is always used for elements that are inside the #qunit-fixture.

How about adding the following logic to assert.t( a, b, c ): if no id is part of parameter b, add #qunit-fixture id to it?

[gibson042]

How about adding the following logic to assert.t( a, b, c ): if no id is part of parameter b, add #qunit-fixture id to it?

That doesn't work; b is documented to as an arbitrary selector, and therefore you can't modify it without tokenizing the whole thing (which doesn't belong in test code). I would not be opposed to making a copy with post-selection filtering semantics, though.

[markelog]

Moved to roadmap