Fix errors on non-browser environments #401

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@twada

twada commented Jan 29, 2013

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 jquery#399

@jzaefferer

This comment has been minimized.

Show comment Hide comment
@jzaefferer

jzaefferer Jan 29, 2013

Member

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.

Member

jzaefferer commented Jan 29, 2013

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

This comment has been minimized.

Show comment Hide comment
@twada

twada Jan 29, 2013

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.

twada commented Jan 29, 2013

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.

@jdalton

This comment has been minimized.

Show comment Hide comment
@jdalton

jdalton Jan 30, 2013

👍

jdalton commented Jan 30, 2013

👍

@Krinkle

This comment has been minimized.

Show comment Hide comment
@Krinkle

Krinkle Jan 30, 2013

Member

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.

Member

Krinkle commented Jan 30, 2013

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

This comment has been minimized.

Show comment Hide comment
@JamesMGreene

JamesMGreene Jan 30, 2013

Member

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

Member

JamesMGreene commented Jan 30, 2013

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

@Krinkle

This comment has been minimized.

Show comment Hide comment
@Krinkle

Krinkle Jan 31, 2013

Member

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!

Member

Krinkle commented Jan 31, 2013

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

This comment has been minimized.

Show comment Hide comment
@twada

twada Jan 31, 2013

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.

twada commented Jan 31, 2013

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

This comment has been minimized.

Show comment Hide comment
@Krinkle

Krinkle Jan 31, 2013

Member

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 ).

Member

Krinkle commented Jan 31, 2013

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

This comment has been minimized.

Show comment Hide comment
@twada

twada Jan 31, 2013

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.

twada commented Jan 31, 2013

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.

@twada twada referenced this pull request Feb 3, 2013

Closed

Regression tests on Node #410

@jzaefferer jzaefferer referenced this pull request Mar 7, 2013

Closed

Split the codebase #378

@JamesMGreene

This comment has been minimized.

Show comment Hide comment
@JamesMGreene

JamesMGreene Mar 19, 2013

Member

@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!

Member

JamesMGreene commented Mar 19, 2013

@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

This comment has been minimized.

Show comment Hide comment
@twada

twada Mar 19, 2013

@JamesMGreene Thanks! I signed. 👌

twada commented Mar 19, 2013

@JamesMGreene Thanks! I signed. 👌

@jdalton

This comment has been minimized.

Show comment Hide comment
@jdalton

jdalton Jul 4, 2013

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

jdalton commented Jul 4, 2013

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

@jzaefferer

This comment has been minimized.

Show comment Hide comment
@jzaefferer

jzaefferer Jul 4, 2013

Member

@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?

Member

jzaefferer commented Jul 4, 2013

@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

This comment has been minimized.

Show comment Hide comment
@jdalton

jdalton Jul 4, 2013

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 )

jdalton commented Jul 4, 2013

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

This comment has been minimized.

Show comment Hide comment
@Krinkle

Krinkle Jul 4, 2013

Member

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.

Member

Krinkle commented Jul 4, 2013

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

This comment has been minimized.

Show comment Hide comment
@jzaefferer

jzaefferer Jul 5, 2013

Member

@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.

Member

jzaefferer commented Jul 5, 2013

@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

This comment has been minimized.

Show comment Hide comment
@JamesMGreene

JamesMGreene Jul 5, 2013

Member

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.

Member

JamesMGreene commented Jul 5, 2013

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

This comment has been minimized.

Show comment Hide comment
@JamesMGreene

JamesMGreene Jul 8, 2013

Member

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.

Member

JamesMGreene commented Jul 8, 2013

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

This comment has been minimized.

Show comment Hide comment
@JamesMGreene

JamesMGreene Jul 9, 2013

Member

Replaced by PR #458.

Member

JamesMGreene commented Jul 9, 2013

Replaced by PR #458.

@moos

This comment has been minimized.

Show comment Hide comment
@moos

moos Sep 21, 2013

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.

moos commented Sep 21, 2013

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

This comment has been minimized.

Show comment Hide comment
@JamesMGreene

JamesMGreene Sep 21, 2013

Member

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

Member

JamesMGreene commented Sep 21, 2013

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

@moos

This comment has been minimized.

Show comment Hide comment
@moos

moos Sep 22, 2013

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.

moos commented Sep 22, 2013

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

This comment has been minimized.

Show comment Hide comment
@JamesMGreene

JamesMGreene Sep 22, 2013

Member

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

Member

JamesMGreene commented Sep 22, 2013

@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