Tests: Add simple tests for Android 2.3 #2509

mgol · 2015-07-29T23:56:16Z

@jquery/core This is an initial stab at simple tests for Android 2.3 but please review what I have before I write tests for other modules.

I've created a separate "module" called basic to contain those selected tests; I also added it to the list run by regular browsers so that we immediately know if those tests are ok.

Note that tests are very basic but testing the whole API even without edge cases (however you define them) would be way too much; if we run more than 10% of assertions in Android 2.3 it will be unstable (older browsers fare better with many modules with few tests in them rather than one module with all). I'd rather remove some CSS tests from this list than add more.

I've temporarily modified our Jenkins jobs to see how that would look:

the regular periodic run without Android 2.3: http://swarm.jquery.org/job/1009
the basic run just for Android 2.3: http://swarm.jquery.org/job/1011

Even these 18 CSS tests seem to sometimes run very slow on Android 2.3: they were running for 11 minutes & timed out, after restarting they passed almost immediately...

dmethvin · 2015-07-30T17:03:41Z

Anything that goes through only basic support doesn't test many code paths at all, yet most of them work with Android 2.3 don't they? It seems like we'd want to instead specifically exclude (and document) the cases we know don't work, like reliableMarginRight. That list of tests could be pulled out into its own module perhaps?

mgol · 2015-07-30T17:27:09Z

Anything that goes through only basic support doesn't test many code paths at all, yet most of them work with Android 2.3 don't they?

Most currently do. The fact that basic won't test many "basic" things is one of the reasons why I'd prefer to just drop Android 2.3 on master and leave it just on compat to avoid such weird compatibility state but only @markelog supported me in this view and the team decided we'll be just running basic tests, i.e. what this PR tries to do.

But yeah, I'm still not terribly convinced.

It seems like we'd want to instead specifically exclude (and document) the cases we know don't work, like reliableMarginRight. That list of tests could be pulled out into its own module perhaps?

Pulling such tests to a separate module would be catering to Android 2.3 too much, IMO; we'd break module separation just because of one browser. IMO it's easier (& better) to just skip problematic tests as we started to do now.

The problem is not only failing tests, though. Those tests run extremely slow, usually ~1/3 of the modules time out and I have to restart those runs a few times before they stabilize and (mostly) pass to be sure we didn't regress. That's quite tedious. Running just very basic tests should at least help with this problem.

gibson042 · 2015-08-03T16:32:53Z

test/unit/basic.js

This already looks like too much... I'd expect to see one test per module (as you have), but no more than 10 assertions per test, and hopefully even fewer. For the CSS module, maybe one assertion like assert.equal( div.css( "width", "50px" ).css( "width" ), "50px", ".css getter/setter" ) and one for .show/.hide and call it done.

mgol · 2015-08-03T20:25:19Z

PR updated (traversing & wrap tests to be written).

mgol · 2015-08-03T20:48:58Z

Ready for final review.

dmethvin · 2015-08-04T02:35:25Z

Now I'm really wanting to get some code coverage stats. :)

gibson042 · 2015-08-04T14:23:11Z

test/unit/basic.js

Could you update these to use assert.async instead of stop/start?

We can do this across the all unit files, so it could be done later.

Sure, since I use the new syntax in these files I may as well get it right.

mgol · 2015-08-04T15:31:41Z

Now I'm really wanting to get some code coverage stats. :)

I had significant problems with getting coverage to be generated in our current setup (most likely I did everything wrong :D), in the end I hacked it around in PhantomJS but excluding jQuery.ajax tests as I had problems with getting them not hang everything and I got:

Statements: 42.14% (1369 / 3249)
Branches: 28.39% (926 / 3262)
Functions: 42.58% (244 / 573)
Lines: 42.14% (1369 / 3249)

markelog · 2015-08-04T15:40:00Z

These stats for all modules? Could run just basic ones?

mgol · 2015-08-04T15:42:45Z

No, just for basic.

mgol · 2015-08-04T15:49:14Z

Travis fails because of problems with installing old jsdom... Can we get #2526 landed ASAP?

EDIT: restarting the job helped. But that's yet another reason that it's not worth automating testing on old jsdom, too much trouble for too little gain.

gibson042 · 2015-08-04T16:17:53Z

This looks nice. 👍

timmywil · 2015-08-04T16:26:28Z

👍

gibson042 · 2015-08-04T16:40:26Z

test/unit/basic.js

You don't need to do the tracking yourself; just use assert.async multiple times (cf. test/unit/deferred.js).

Updated, although that doesn't save that much. Ideally assert.async() would accept a parameter indicating how many async operations are expected and that would require calling done() this many times.

BTW, done.pop().call() is not needed, it's just done.pop()() after all.

Of course you're right about .call(), and I agree on the former point. In fact, I meant to create a ticket for that earlier, but it fell off my radar. Anyway, corrected now: qunitjs/qunit/issues/843

Thanks for creating a ticket, I should've done it myself. :)

Fixes gh-2505 Refs gh-2483

Commit 2c7e9c9 added the basic test suite; these are the only tests that are now run on Android 2.3 on master. On compat we're keeping full Android 2.3 support for now but the tests and the testswarm basic run mode have been cherry-picked anyway to reduce the divergence between branches. (cherry-picked from 2c7e9c9) Fixes gh-2505 Closes gh-2509 Refs gh-2483

jquerybot added the CLA: Valid label Jul 29, 2015

mgol force-pushed the simple-tests branch from 09c2ff8 to 626b801 Compare July 29, 2015 23:56

mgol added 2.x-only Needs review Support and removed 2.x-only Support labels Jul 29, 2015

mgol mentioned this pull request Jul 30, 2015

Add basic tests for Android 2.3 on master, drop full support #2505

Closed

gibson042 reviewed Aug 3, 2015
View reviewed changes

mgol force-pushed the simple-tests branch 2 times, most recently from 7a36083 to 8d090d4 Compare August 3, 2015 20:25

mgol force-pushed the simple-tests branch from 8d090d4 to 11d0627 Compare August 3, 2015 20:48

mgol changed the title ~~[WIP]: Add simple tests for Android 2.3~~ Tests: Add simple tests for Android 2.3 Aug 3, 2015

gibson042 reviewed Aug 4, 2015
View reviewed changes

Tests: Add simple tests for Android 2.3

7c40bbe

Fixes gh-2505 Refs gh-2483

mgol force-pushed the simple-tests branch from acda1d6 to 7c40bbe Compare August 5, 2015 12:49

mgol removed the Needs review label Aug 17, 2015

mgol closed this in 2c7e9c9 Sep 8, 2015

mgol deleted the simple-tests branch September 8, 2015 16:07

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Tests: Add simple tests for Android 2.3 #2509

Tests: Add simple tests for Android 2.3 #2509

Uh oh!

Conversation

mgol commented Jul 29, 2015

Uh oh!

dmethvin commented Jul 30, 2015

Uh oh!

mgol commented Jul 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgol commented Aug 3, 2015

Uh oh!

mgol commented Aug 3, 2015

Uh oh!

dmethvin commented Aug 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgol commented Aug 4, 2015

Uh oh!

markelog commented Aug 4, 2015

Uh oh!

mgol commented Aug 4, 2015

Uh oh!

mgol commented Aug 4, 2015

Uh oh!

gibson042 commented Aug 4, 2015

Uh oh!

timmywil commented Aug 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants