Build: Don't assume the browser environment; smoke test on Node w/ jsdom #1949

mgol · 2014-12-17T21:44:48Z

Fixes gh-1950

mgol · 2014-12-17T21:46:02Z

+26 bytes currently; perhaps that can be improved.

EDIT: +35 bytes now.

Final EDIT: the landed commit ended up at +32 bytes.

timmywil · 2014-12-17T21:46:47Z

src/ajax/parseXML.js

@@ -11,7 +11,8 @@ jQuery.parseXML = function( data ) {

 	// Support: IE9
 	try {
-		tmp = new DOMParser();
+		// jQuery.parseXML is browser-only
+		tmp = new DOMParser(); // jshint ignore: line


DOMParser should just be added to globals.

If we add DOMParser to globals and use it somewhere on the top scope, we'll break Node & other environments that don't have it. That's why I went for explicitly marking places that are browser-only; I think it's safer (and there are only two such places on master).

I just don't like jshint comments. What about a jshintrc in the ajax folder?

I don't like it either. :) But it seemed as a necessary evil.

We don't know if something in the ajax folder isn't run immediately when including jQuery. We know this about parseXML and the xhr transport but this seems dangerous to do globally for ajax.

I'm open for better suggestions; I just don't see them currently.

How about creating a subdirectory src/ajax/var/browser-only/ with vars for DOMParser & XMLHttpRequest and put such a .jshintrc with browser: true there?

Works for me.

@timmywil We could also use window.DOMParser etc. which has the advantage that if jsdom ever gets support for that it starts to work.

In fact, jsdom already supports XMLHttpRequest. I'll modify the PR.

Edit: done.

timmywil · 2014-12-17T21:48:13Z

src/core/support.js


 support.createHTMLDocument = (function() {
 	var doc = document.implementation.createHTMLDocument( "" );
+	// Support: Node with jsdom<=1.5.0+
+	// jsdom's document created via the above method doesn't contain the body
+	if (!doc.body) {


Spacing. Did jscs not pick this up?

It didn't. :/

timmywil · 2014-12-17T21:52:33Z

node-smoke-tests.js

+
+require( "jsdom" ).env( "", function (errors, window) {
+	if (errors) {
+		console.error(errors);


Spaces here too.

This file might need to be added to the Gruntfile.

Right; this is taken from my commit (e1303a3) at the standard-then-tests branch where the external file was actually necessary.

BTW, when I inline it spawn isn't even needed.

mgol · 2014-12-18T00:00:24Z

src/.jshintrc

+		"setTimeout": true,
+		"clearTimeout": true,
+		"setInterval": true,
+		"clearInterval": true,


jsdom supports setTimeout etc.; they wrap Node's setTimeout but also cache results in order to auto-clear timeouts/intervals on window.close(). This seems to be an extremely edge-case scenario and I envision we might want to support simpler than jsdom replacements for window so it's better to use global setTimeout.

Thoughts?

@denis-sokolov claims it's a niche so I tend to agree. :)

So it seems safe to just use setTimeout and the others global then? if node and browsers both support it global I think it should be safe.

dmethvin · 2014-12-18T02:09:03Z

@mzgol I grabbed gh-1788 since it was specifically about this bug and moved it to the current release. Seemed good to have a ticket about it. 😸 We can make another issue with a general description since this is much more comprehensive.

dmethvin · 2014-12-18T02:15:55Z

src/core/support.js


 support.createHTMLDocument = (function() {
 	var doc = document.implementation.createHTMLDocument( "" );
+	// Support: Node with jsdom<=1.5.0+
+	// jsdom's document created via the above method doesn't contain the body


Wow, didn't we pull out something similar for framed docs? There are no new bugs it seems.

mgol · 2014-12-18T09:30:32Z

@mzgol I grabbed gh-1788

@dmethvin OK, I've created #1950 instead.

gibson042 · 2014-12-18T14:00:37Z

Gruntfile.js

 	// Short list as a high frequency watch task
 	grunt.registerTask( "dev", [ "build:*:*", "lint" ] );

 	// Default grunt
-	grunt.registerTask( "default", [ "jsonlint", "dev", "uglify", "dist:*", "compare_size" ] );
+	grunt.registerTask( "default",
+		[ "jsonlint", "dev", "uglify", "dist:*", "compare_size", "node-smoke-test" ] );


I'd prefer to use the build/dist/test pattern that we have in other projects (e.g., sizzle, qunit). In other words, …, "dist:*", "node-smoke-test", "compare_size", or better still, moving "node-smoke-test" into a "test" task.

better still, moving "node-smoke-test" into a "test" task.

I had this in one version of a patch but this would be the only thing in the test task so far.

We'll also need to change the npm test script to do grunt test instead of just grunt.

Is that what you want?

We'll need to update Jenkins jobs to use npm test instead of grunt what they do now. I'll do it.

Done & done.

gibson042 · 2014-12-18T14:07:29Z

I like it. 👍

gibson042 · 2014-12-18T16:33:30Z

Gruntfile.js

 	// Short list as a high frequency watch task
 	grunt.registerTask( "dev", [ "build:*:*", "lint" ] );

-	// Default grunt
+	grunt.registerTask( "test", [ "default", "node-smoke-test" ] );


Shouldn't that go the other way (default including test)?

Ah, that's what you meant.

So, just to be sure:

grunt.registerTask( "test", [ "node-smoke-test" ] ); grunt.registerTask( "default", [ "jsonlint", "dev", "uglify", "dist:*", "test", "compare_size" ] );

That will mean that compare_size won't run if test fails. Is that OK?

I'm fine with it, not least of all because it's consistent with Sizzle. And if that changes in the future, it should change everywhere.

I was thinking about it but I'm not convinced that's the best approach. In Core grunt is so far linting and building the project and that's it. Current smoke-testing is short but we'll be adding Promises/A+ compliance test (see e1303a3) which is not quick so I doubt we want to auto-run it just when we just want to build the files.

We can run Node smoke-testing in the default task as it's quick but putting it in the test task will IMO mean we'll need to refactor it again in the nearby future.

Thoughts? cc @markelog @dmethvin @timmywil

I trust your opinion more than mine here :)

Do we really need to add new task to default task? Those tests are for jsdom compliance, do we really need every time we want to build it?

We need to run grunt testswarm on Jenkins explicitly as it needs parameters, see config of this job:

Don't have access to it, you mean something like this -
grunt testswarm:d6c97abb744bfe8c67ab9158aecdb5bb1c05e47b:/usr/local/bin/tools/node-testswarm-config.json:jquery:jquery?

We could you use new npm feature - passing arguments to npm scripts, although it doesn't look pretty.

@markelog

Do we really need to add new task to default task? Those tests are for jsdom compliance, do we really need every time we want to build it?

They're rather fast, though (500 ms on my machine) and they prevent us from accidentally commiting code breaking compat with Node. We do run linters on each grunt run for example.

On the other hand, the jsHint setup when changed like in this PR will catch most Node incompatibilities so maybe it's not so important to check it all the time.

I'll go by the following solution:

npm run build # does npm install && grunt npm test # does grunt test grunt testswarm:sthsthsth

I'll land this now without running Node smoke tests on grunt; we can revisit later. I want this on master ASAP, though so that we're covered agains past mistakes from now on.

I'd be in favor of running the tests all the time, but that's just me.

@jaubourg They require jsdom presence, though and some people may have a problem with that (see my comment: #1949 (comment)). I'm not sure if we have a perfect solution here without optionalDevDependencies but that's just one thing to consider.

mgol · 2014-12-22T20:25:01Z

I was made aware by email that:

jsdom requires node-gyp which - in turn - requires Python & some other build stuff
No other dev dependency of ours requires those tools
Being able to run npm install is necessary to be able to build a custom version.

So it seems we shouldn't just flat out fail if jsdom fails to install as it's not required by the build process. I'd normally just put it in optionalDevDependencies but they don't exist (see npm/npm#3870 - @othiym23, you have a real use case here ;)).

The only way to resolve it that I see is to create an optionalDevDependencies key, move jsdom there and install the specified version optionally in the postinstall hook that will be forgiving if the installation fails. A little ugly.

Any better ideas?

EDIT: I still want to land this PR even if we don't figure out how to solve the presented problem soon; we can figure it out later but it still seems important.

Fixes jquerygh-1950

Fixes gh-1950 Closes gh-1949

mgol · 2014-12-26T12:30:19Z

Landed. I went for running the tests via:

npm install
npm test
grunt testswarm:other:params:here

In this was we can run tests in the same way on master and compat; also, Travis runs npm install && npm test by default so we also get that covered.

npm test on master runs grunt test and on compat: grunt. The test task runs default and then test_fast.

@jaubourg grunt will not run Node smoke tests for you (until we decide otherwise) but npm test and grunt test do so you can use one of them instead. If you miss, Travis has you covered.

mgol · 2014-12-27T01:28:05Z

I've looked a little at jsdom build requirements and they'll all be gone once Node 0.12 comes out (which, cough, should be soon): jsdom/jsdom#683 (comment)

Judging by how complicated and fragile any workaround would be here and considering that the problem will disappear in some time I'd rather leave it as is.

Fixes jquerygh-2501 Closes jquerygh-2504 Refs jquerygh-1950 Refs jquerygh-1949 Refs jquerygh-2397 Refs jquerygh-1537 Refs 842958e

Fixes jquerygh-2133 Fixes jquerygh-2501 Closes jquerygh-2504 Refs jquerygh-1950 Refs jquerygh-1949 Refs jquerygh-2397 Refs jquerygh-1537 Refs 842958e

Fixes jquerygh-2133 Fixes jquerygh-2501 Closes jquerygh-2504 Refs jquerygh-1950 Refs jquerygh-1949 Refs jquerygh-2397 Refs jquerygh-1537 Refs jquerygh-2504 Refs 842958e

Fixes gh-1950 Closes gh-1949

timmywil reviewed Dec 17, 2014
View reviewed changes

mgol mentioned this pull request Dec 17, 2014

2.1.2-pre, ajax.js: environments that don't have 'location' variable will break #1788

Closed

timmywil reviewed Dec 17, 2014
View reviewed changes

mgol force-pushed the node-tests branch from 3c00bcf to b2fcb12 Compare December 17, 2014 21:49

timmywil reviewed Dec 17, 2014
View reviewed changes

mgol force-pushed the node-tests branch 6 times, most recently from d6e7265 to f91bc4a Compare December 17, 2014 22:53

mgol reviewed Dec 18, 2014
View reviewed changes

mgol force-pushed the node-tests branch from f91bc4a to 8acd10f Compare December 18, 2014 00:45

dmethvin reviewed Dec 18, 2014
View reviewed changes

mgol force-pushed the node-tests branch from 8acd10f to f0b5ad7 Compare December 18, 2014 09:24

mgol self-assigned this Dec 18, 2014

gibson042 reviewed Dec 18, 2014
View reviewed changes

mgol force-pushed the node-tests branch 2 times, most recently from 30b61b5 to 3143efc Compare December 18, 2014 16:07

gibson042 reviewed Dec 18, 2014
View reviewed changes

This was referenced Dec 26, 2014

Ajax: simplify XHR detection. #1975

Closed

Simplify XHR detection #1967

Closed

Build: Don't assume the browser environment; smoke test on Node w/ jsdom

5a3f914

Fixes jquerygh-1950

mgol force-pushed the node-tests branch from 3143efc to 5a3f914 Compare December 26, 2014 12:22

mgol closed this Dec 26, 2014

mgol deleted the node-tests branch December 26, 2014 12:27

mgol added a commit that referenced this pull request Dec 26, 2014

Build: Don't assume the browser environment; smoke test on Node w/ jsdom

76df9e4

Fixes gh-1950 Closes gh-1949

gibson042 mentioned this pull request Dec 30, 2014

Build: Rearrange grunt/npm tasks into a build/dist/test pattern #1980

Closed

gibson042 mentioned this pull request Jun 16, 2015

Core: Use window.setTimeout & friends instead of global equivalents #2397

Merged

mgol mentioned this pull request Jul 28, 2015

Core: Support non-browser environments #2504

Merged

mgol added a commit to mgol/jquery that referenced this pull request Jul 28, 2015

Core: Support non-browser environments

d79dbf0

Fixes jquerygh-2501 Closes jquerygh-2504 Refs jquerygh-1950 Refs jquerygh-1949 Refs jquerygh-2397 Refs jquerygh-1537 Refs 842958e

mgol added a commit to mgol/jquery that referenced this pull request Jul 28, 2015

Core: Support non-browser environments

a99ed3c

Fixes jquerygh-2133 Fixes jquerygh-2501 Closes jquerygh-2504 Refs jquerygh-1950 Refs jquerygh-1949 Refs jquerygh-2397 Refs jquerygh-1537 Refs 842958e

mgol added a commit that referenced this pull request Nov 10, 2015

Build: Don't assume the browser environment; smoke test on Node w/ jsdom

115b2bb

Fixes gh-1950 Closes gh-1949

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

Build: Don't assume the browser environment; smoke test on Node w/ jsdom #1949

Build: Don't assume the browser environment; smoke test on Node w/ jsdom #1949

Conversation

mgol commented Dec 17, 2014

mgol commented Dec 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmethvin commented Dec 18, 2014

Choose a reason for hiding this comment

mgol commented Dec 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 commented Dec 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgol commented Dec 22, 2014

mgol commented Dec 26, 2014

mgol commented Dec 27, 2014