Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix errors on non-browser environments #401

Closed
wants to merge 1 commit into from

6 participants

@twada

Fix errors on non-browser environments since that does not have addEventListener nor attachEvent.

Since QUnit 1.11.0, an error occurs when loading QUnit under non-browser environments. For example, on Node.js, require('qunitjs') raises an error.

Stacktrace on Node.js

/Users/takuto/work/git-sandbox/github/qunit-tap/test/compatibility/1.11.0/qunit.js:1506
        elem.attachEvent( "on" + type, fn );
             ^
TypeError: Object #<Object> has no method 'attachEvent'
    at addEvent (/Users/takuto/work/git-sandbox/github/qunit-tap/test/compatibility/1.11.0/qunit.js:1506:8)
    at /Users/takuto/work/git-sandbox/github/qunit-tap/test/compatibility/1.11.0/qunit.js:1184:1
    at Object.<anonymous> (/Users/takuto/work/git-sandbox/github/qunit-tap/test/compatibility/1.11.0/qunit.js:2152:2)
    at Module._compile (module.js:449:26)
    at Object.Module._extensions..js (module.js:467:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:362:17)
    at require (module.js:378:17)
    at Object.<anonymous> (/Users/takuto/work/git-sandbox/github/qunit-tap/test/node/test_helper.js:18:13)

This PR is a refinement of closed PR #399

@jzaefferer
Owner

This is actually a fix for the regression caused by f249711

Need to check if not calling QUnit.load at all is actually the desired behavior in non-browser environments.

@twada

As a citizen of non-browser world, I'd prefer calling QUnit.load or QUnit.start explicitly after setting proper QUnit.config values and some logging callbacks. This style makes QUnit behaviour on non-browser deterministic.

@Krinkle
Collaborator

Code looks good, but I'll defer merging this until we have a basic test suite for non-browser runs. We should probably use grunt-contrib-qunit or the phantomjs add-on.

@JamesMGreene
Collaborator

Those are both browser-based, though...? You probably want to use something more like node-qunit.

@Krinkle
Collaborator

Indeed, we already have phantomjs tests in grunt, which we run on every commit from Jenkins. We've had this for several months now.

Don't know what I was thinking.. node-qunit indeed!

@twada

I think we need to test QUnit HEAD every time to detect regressions immediately.
It can be simply done by adding a new task to grunt.js. For example,

grunt.registerTask( "test-on-node", function() {
    var done = this.async();
    var QUnit = require("./qunit/qunit");
    QUnit.done(function( details ) {
        grunt.log.write(details.total + " assertions (" + details.runtime + "ms)");
        done(details.failed === 0);
    });
    QUnit.config.autorun = false;
    QUnit.load();

    QUnit.module('QUnit assertions');
    QUnit.test('truthy', function (assert) {
        assert.ok(true);
    });
    QUnit.test('equality', function (assert) {
        assert.equal('foo', 'foo');
    });
});

I'll request another PR about this if possible.

@Krinkle
Collaborator

I'd prefer to run the entire test suite instead of just a few basic assertions.

It's quite simple:

  • Depend and require on npm module qunit (source) - which is a small nodejs wrapper around QUnit
  • Call qunit.run( .. )

See also travis-ci-node-qunit for a working example.

Actually, node-qunit doesn't call QUnit.load anywhere. I'm not sure if it works when it isn't called, but assuming it relied on addEvent calling it immediately, we should probably call QUnit.load in an else case of the if ( defined.document ).

@twada

we should probably call QUnit.load in an else case of the if ( defined.document ).

On non-browser environments, I think calling QUnit.load immediately in an else of the if ( defined.document ) is not a good idea because...

  • We don't have any chance to customize QUnit.config values before QUnit.load.
  • QUnit.begin event cannot be handled (begin event is fired before registering logging callbacks)

When running under browsers (including PhantomJS), QUnit.load is called on window.load, so there is a slight delay to customize configs and register logging callbacks.

This was referenced
@JamesMGreene
Collaborator

@twada: You need to sign the CLA before we can work on merging this PR. Please add a comment here when you've done so. Thanks!

@twada

@JamesMGreene Thanks! I signed. :ok_hand:

@JamesMGreene JamesMGreene referenced this pull request from a commit
@JamesMGreene JamesMGreene Adding Node.js support!
 - Merge branch 'dont_addevent_on_load_under_nonbrowser' of
   https://github.com/twada/qunit. Closes PR #401.
67689d5
@JamesMGreene JamesMGreene referenced this pull request from a commit
@JamesMGreene JamesMGreene Enabling Node.js support.
Closes #401.
Closes #410.
8db51fc
@jdalton

The v1.12.0 release breaks with my use because it doesn't export things like QUnit.jsDump, QUnit.config, and QUnit.init.

@jzaefferer
Owner

@jdalton we didn't change the exports between 1.11 and 1.12 - your comment sounds like 1.12 has regressions. Did it work for you with 1.11? Or previous versions?

@jdalton

Yes 1.11.0 worked because it was exported as extend( exports, QUnit ) in 1.12.0 it's exported as extend( exports, QUnit.constructor.prototype )

@Krinkle
Collaborator

We shouldn't export those detached as globals, but they should be available through the QUnit object. This distinction is clear in the way we export for the browser, but there's it's not so clear in the export for modules as 1) there's a line missing there, 2) the regular export of the subset don't looks like detached properties when you use them (var QUnit = require('qunit'); QUnit.test( .. );)

    extend( window, QUnit.constructor.prototype );
    window.QUnit = QUnit;
    extend( exports, QUnit.constructor.prototype );

This should have a:

    exports.QUnit = QUnit;

That way you'll have access to the proper object (to access like var QUnit = require('qunit').QUnit) with a better separation and preservation of what are direct own properties and which are inherited (with regards to our backwards compatibility hack for the callback functions) and whatever other stuff we do.

@jzaefferer
Owner

@JamesMGreene can you push your branch into this repo and replace this PR with a new one? Reference this PR and whatever other tickets, add tests for the issue @jdalton brought up.

I'm still not sure why we even changed the exports, I didn't see any mention of that in the commit log since 1.11. Too late anyway, so let's get it fixed and prevent future regressions.

@JamesMGreene
Collaborator

For Node.js consumers, when we split the codebase in future releases (per #378), we could just tell them to import such functions via require('qunitjs/utils').jsDump instead of require('qunitjs').jsDump to eliminate public exposure of non-API methods. Not positive if that works in other CommonJS module loaders... @jdalton?

Yes, I'll try to update my branch and submit a new PR today.

@JamesMGreene
Collaborator

Sorry, forgot I had a wedding that night. As for regression testing @jdalton's issue, should the test(s) effectively just verify that those utility functions are exported, or do something more?

I will plan to add test(s) and push a new PR tonight, still listing @twada as the author.

@JamesMGreene JamesMGreene referenced this pull request from a commit
@twada twada Adding a new Grunt task to enable Node.js testing.
Closes #401.
Closes #410.
e4a6db3
@JamesMGreene
Collaborator

Replaced by PR #458.

@moos

I'm a little lost in the discussion. I have v1.12.0 and just ran into this issue (the original issue opened by @twada). I guess this fix didn't make it to v1.12.0, right?

@twada's fix was simple enough to allow drop-in node.js support. Now I have to either patch or look for an alternative. Thanks.

@JamesMGreene
Collaborator

@moos We would welcome a compliant patch from you that takes care of all the use cases that weren't previously covered. :+1:

@moos

I hacked my way through to get QUnit & node.js working for me. This includes @twada's patch above and this bit to address @jdalton's issue that I also ran into:

if ( typeof exports !== "undefined" ) {
extend( exports, QUnit);
extend( exports, QUnit.constructor.prototype);
}

My test file looks like:

QUnit.config.reorder = false;
QUnit.config.autorun = false; // !important to run tests in sequence!

QUnit.test('test 1', function(assert){ ... });
...
QUnit.test('test N', function(assert){ ... });

QUnit.load();

Also overrode some callbacks (QUnit.log, etc.) per test/node-test.js to get logging. Overall a painful experience :) I love QUnit, but don't think it's ready as a drop-in module in a node.js environment yet.

@JamesMGreene
Collaborator

@moos: Which is exactly why PR #458 is, unfortunately, still in progress. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 29, 2013
  1. @twada
This page is out of date. Refresh to see the latest.
Showing with 4 additions and 1 deletion.
  1. +4 −1 qunit/qunit.js
View
5 qunit/qunit.js
@@ -21,6 +21,7 @@ var QUnit,
// Keep a local reference to Date (GH-283)
Date = window.Date,
defined = {
+ document: typeof window.document !== "undefined",
setTimeout: typeof window.setTimeout !== "undefined",
sessionStorage: (function() {
var x = "qunit-test-string";
@@ -1181,7 +1182,9 @@ QUnit.load = function() {
}
};
-addEvent( window, "load", QUnit.load );
+if ( defined.document ) {
+ addEvent( window, "load", QUnit.load );
+}
// `onErrorFnPrev` initialized at top of scope
// Preserve other handlers
Something went wrong with that request. Please try again.