Skip to content

Conversation

@mgol
Copy link
Member

@mgol mgol commented May 20, 2019

Summary

This PR ports Sizzle tests to jQuery. Reviewing by commit might be the best. Sizzle tests are located in three files named extending.js, utilities.js & selector.js. TODO list:

  • Review tests from extending.js: these are only tests for custom pseudos which we'll be removing so I haven't backported them.
  • Review tests from utilities.js: I backported everything that wasn't already present with the exception of Sizzle.attr a.k.a. jQuery.find.attr. This is an internal API & I think jQuery attribute tests are thorough enough that we don't need to add these tests. Let me know if you disagree.
  • Review tests from selector.js.

Checklist

@mgol mgol added the Tests label May 20, 2019
@mgol mgol added this to the 4.0.0 milestone May 20, 2019
@mgol mgol force-pushed the sizzle-tests-backport branch from d3a7cab to d459aad Compare May 27, 2019 19:56
@mgol
Copy link
Member Author

mgol commented May 27, 2019

I ported many more tests, pushed in new commits. Still a lot to go but we're getting closer.

@mgol
Copy link
Member Author

mgol commented Jun 9, 2019

I added a few more commits. It's not 100% ready yet but it's close so I'd appreciate a review.

I'll finish remaining pieces & address feedback when I'm back from vacation.

@timmywil
Copy link
Member

This is a lot to review. Honestly, I'm okay with merging when you feel it is ready and then we can increment in smaller PRs from there.

@mgol mgol marked this pull request as ready for review June 24, 2019 18:22
@mgol
Copy link
Member Author

mgol commented Jun 24, 2019

As discussed at the meeting today, I'll merge it soon (tomorrow or on Wednesday) unless someone wants to have a look.

Some final remarks/questions from me (you can review them instead if you want):

  1. I updated selector-native slightly so that it passes some backported tests it used to fail - some Sizzle logic wasn't backported there.
  2. Re test/unit/extending.js: it contains tests for custom pseudos which we'll soon drop so I haven't backported it
  3. Re test/unit/utilities.js: it tests a few APIs that we already have tets for on the jQuery side; it also contains tests for Sizzle.attr, though. I assume they're not needed as jQuery attr tests should be enough.
  4. Tests using Sizzle.matchesSelector have been migrated by replacing Sizzle.matchesSelector(selector, elem) by jQuery(elem).is(selector). Sizzle.matches tests weren't backported as it's a private API that no code in jQuery or Sizzle uses.
  5. Lots of tests using Sizzle have been migrated to use jQuery.find - should we document that method? If not, should we migrate those tests to use jQuery? Some queries need to be modified then. jQuery.find accepts as many as four parameters; do we want to support them all? Some tests are using all of them right now.
  6. Safari has surprisingly good support for CSS selectors that other browsers don't support. E.g. :not(complex selector) works in Safari. Therefore, I UA-sniff in tests & run some tests just in Safari OR if Sizzle is present (by checking for jQuery.find.compile as existing tests do).

Apart from porting most Sizzle tests to jQuery (mostly to its selector module),
this commit fixes selector-native so that a jQuery custom compilation that
excludes Sizzle passes all tests as well.
@mgol mgol force-pushed the sizzle-tests-backport branch from 22ad604 to 2dda538 Compare June 26, 2019 18:48
@mgol mgol merged commit 79b74e0 into jquery:master Jun 26, 2019
@mgol mgol removed the Needs review label Jun 26, 2019
@mgol mgol deleted the sizzle-tests-backport branch June 26, 2019 19:39
@lock lock bot locked as resolved and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Development

Successfully merging this pull request may close these issues.

2 participants