Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

export.js: Export in all CommonJS environments #540

Closed
wants to merge 1 commit into from
Closed

export.js: Export in all CommonJS environments #540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .jshintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

"globals": {
"QUnit": false,
"exports": false,
"module": false
}
}
9 changes: 7 additions & 2 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ language: node_js
node_js:
- "0.10"
before_install:
- npm install -g grunt-cli
- "npm install -g grunt-cli"
- "sudo mkdir /opt/rhino-1.7R5 && sudo wget -O /opt/rhino-1.7R5/js.jar https://oss.sonatype.org/content/repositories/snapshots/org/mozilla/rhino/1.7R5-SNAPSHOT/rhino-1.7R5-20120629.144839-4.jar"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's extract these into a grunt task or separate script so that developers working on QUnit can use this to install ringo and rhino locally as well. I copied them manually and ran into an issue...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with that, I think I will download the files over to .envs locally in the grunt task. Should I do this using grunt-shell all use node's native modules to do it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to use grunt.util.spawn, as mentioned below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@D10 installing the other envs is still a blocker for this. Did you ever look into that install grunt task?

- "echo -e '#!/bin/sh\\njava -jar /opt/rhino-1.7R5/js.jar $@' | sudo tee /usr/local/bin/rhino && sudo chmod +x /usr/local/bin/rhino"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this line to work when copy&pasting to the terminal I had to put an $ at the front of the echoed string, like this:

echo -e $'#!/bin/sh\\njava -jar /opt/rhino-1.7R5/js.jar $@' | ...

- "wget http://ringojs.org/downloads/ringojs-0.9.zip && sudo unzip ringojs-0.9 -d /opt && rm ringojs-0.9.zip"
- "sudo ln -s /opt/ringojs-0.9/bin/ringo /usr/local/bin/ringo && sudo chmod +x /usr/local/bin/ringo"
- "export PATH=\"$PATH:/usr/local/bin\""
script:
- npm run-script ci
- "npm run-script ci"
74 changes: 29 additions & 45 deletions Gruntfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
/*jshint node:true */
module.exports = function( grunt ) {

var path = require( "path" ),
spawn = require( "child_process" ).spawn,
envs = [ "node", "rhino", "ringo" ];

require( "load-grunt-tasks" )( grunt );

function process( code, filepath ) {
Expand Down Expand Up @@ -148,54 +152,34 @@ grunt.registerTask( "testswarm", function( commit, configFile, projectName, brow
});
});

// TODO: Extract this task later, if feasible
// Also spawn a separate process to keep tests atomic
grunt.registerTask( "test-on-node", function() {
var testActive = false,
runDone = false,
done = this.async(),
QUnit = require( "./dist/qunit" );

QUnit.testStart(function() {
testActive = true;
});
QUnit.log(function( details ) {
if ( !testActive || details.result ) {
return;
}
var message = "name: " + details.name + " module: " + details.module +
" message: " + details.message;
grunt.log.error( message );
});
QUnit.testDone(function() {
testActive = false;
});
QUnit.done(function( details ) {
if ( runDone ) {
return;
}
var succeeded = ( details.failed === 0 ),
message = details.total + " assertions in (" + details.runtime + "ms), passed: " +
details.passed + ", failed: " + details.failed;
if ( succeeded ) {
grunt.log.ok( message );
} else {
grunt.log.error( message );
}
done( succeeded );
runDone = true;
envs.forEach( function( env ) {
grunt.registerTask( "test-on-" + env, function() {
var process = global.process,
oldWorkingDir = process.cwd(),
done = this.async();

process.chdir( path.join( oldWorkingDir, "test" ) );
grunt.log.ok( "Testing on " + env );

grunt.util.spawn( {
cmd: env,
args: [ "env-tests.js" ]
}, function( error, result, code ) {
if ( code ) {
grunt.log.error( "Failed tests on " + env );
grunt.log.error( result );
} else {
grunt.log.ok( "Passed tests on " + env );
grunt.log.ok( result );
}
process.chdir( oldWorkingDir );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be nothing after done(), swap these lines. Unless it's this way on purpose, in which case I'm curious to learn why.

done( code === 0 ? true : false );
});
});
QUnit.config.autorun = false;

require( "./test/logs" );
require( "./test/test" );
require( "./test/deepEqual" );
require( "./test/globals" );

QUnit.load();
});

grunt.registerTask( "build", [ "concat" ] );
grunt.registerTask( "default", [ "build", "jshint", "jscs", "qunit", "test-on-node" ] );
grunt.registerTask( "test-on-envs", envs.map( function( env ) { return "test-on-" + env; }) );
grunt.registerTask( "default", [ "build", "jshint", "jscs", "qunit", "test-on-envs" ] );

};
25 changes: 16 additions & 9 deletions src/export.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
// For browser, export only select globals
if ( typeof window !== "undefined" ) {
if ( typeof window !== "undefined" ||
(
typeof exports === "object" && exports && !exports.nodeType &&
typeof module === "object" && module && module.exports !== exports
) ) {

// Deprecated
// Extend assert methods to QUnit and Global scope through Backwards compatibility
Expand All @@ -19,11 +23,10 @@ if ( typeof window !== "undefined" ) {
}
})();

(function() {
(function( exports ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? Can't you just declare a variable (with a different name) that gets assigned the argument of this function?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IIFE already existed before this PR.

var i, l,
keys = [
"test",
"module",
"expect",
"asyncTest",
"start",
Expand All @@ -41,14 +44,18 @@ if ( typeof window !== "undefined" ) {
];

for ( i = 0, l = keys.length; i < l; i++ ) {
window[ keys[ i ] ] = QUnit[ keys[ i ] ];
exports[ keys[ i ] ] = QUnit[ keys[ i ] ];
}
})();

window.QUnit = QUnit;
}
if ( exports === window ) {
exports.module = QUnit.module;
}

exports.QUnit = QUnit;
})( typeof exports === "object" ? exports : window );

// For CommonJS environments, export everything
if ( typeof module !== "undefined" && module.exports ) {
// For Node.js, export everything
} else if ( typeof window === "undefined" ) {
module.exports = QUnit;
QUnit.QUnit = QUnit;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still can't tell what the purpose of this is.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For envs without module.export support, we need to export the whole QUnit object, so properties like QUnit.config can be accessed.

}
65 changes: 65 additions & 0 deletions test/env-tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/* jshint node:true */
/* global define:true */
;( function() {

var root = typeof global === "object" && global || this,
load = ( typeof require === "function" && !( root.define && define.amd ) ) ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to extract this into a (factory) function that uses less tertiaries and boolean grouping, to make the thing more readable?

require :
( !root.document && root.java && root.load ) ||
function() { throw Error( "Unable to load module" ); },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should throw an error immediately, not just when the load function is invoked eventually.

log = typeof root.print === "function" && root.print || root.console.log,
process = root.phantom || root.process,
java = root.java,
testActive = false,
QUnit = load( "../dist/qunit.js" );

QUnit = root.QUnit = ( QUnit ? QUnit.QUnit : root.QUnit );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is rather indecipherable. It looks related to the QUnit.QUnit assignment above, but I can't tell why its necessary, if at all.


QUnit.testStart( function() {
testActive = true;
});
QUnit.log( function( details ) {
if ( !testActive || details.result ) {
return;
}
var message = "name: " + details.name + " module: " + details.module +
" message: " + details.message;
log( message );
});
QUnit.testDone( function() {
testActive = false;
});
QUnit.done( function( details ) {
var succeeded = ( details.failed === 0 ),
message = details.total + " assertions in (" + details.runtime + "ms), passed: " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing spaces

details.passed + ", failed: " + details.failed;
if ( succeeded ) {
log( "Passed: " + message );
} else {
log( "Failed: " + message );
}

// exit out of Node.js or PhantomJS
try {
process.exit( succeeded ? 0 : 1 );
} catch ( e ) { }

// exit out of Rhino, or RingoJS
try {
if ( succeeded ) {
root.quit();
} else {
java.lang.System.exit( 1 );
}
} catch ( e ) { }
});
QUnit.config.autorun = false;

load( "./logs.js" );
load( "./test.js" );
load( "./deepEqual.js" );
load( "./globals.js" );

QUnit.load();

}() );
12 changes: 7 additions & 5 deletions test/globals.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*global ok: false, equal: false */
/*global ok: false, equal: false, global: false */
(function( window ) {

QUnit.module( "globals" );
Expand Down Expand Up @@ -33,10 +33,14 @@ QUnit.test( "QUnit object", function( assert ) {

QUnit.test( "QUnit exported methods", function( assert ) {
var globals = [
"test", "asyncTest", "module",
"test", "asyncTest",
"start", "stop"
];

if ( !( typeof exports === "object" && exports && typeof module === "object" && module ) ) {
globals.push( "module" );
}

// 2 assertions per item on checkExported
assert.expect( globals.length * 2 );

Expand All @@ -58,6 +62,4 @@ QUnit.test( "Exported assertions", function() {
});

// Get a reference to the global object, like window in browsers
}( (function() {
return this;
}.call()) ));
}( typeof global === "object" && global || this ));
27 changes: 0 additions & 27 deletions test/narwhal-test.js
View file
Open in desktop

This file was deleted.

27 changes: 0 additions & 27 deletions test/node-test.js
View file
Open in desktop

This file was deleted.

14 changes: 7 additions & 7 deletions test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
/* global global:true */
(function( window ) {

var OrgDate, state;
var OrgDate, state,
document = window.document;

function getPreviousTests( rTestName, rModuleName ) {
var testSpan, moduleSpan,
Expand Down Expand Up @@ -85,7 +87,7 @@ QUnit.test( "module with setup, expect in test call", function( assert ) {
});

// TODO: More to the html-reporter test once we have that.
if ( typeof document !== "undefined" ) {
if ( document ) {

QUnit.module( "<script id='qunit-unescaped-module'>'module';</script>", {
setup: function() {
Expand Down Expand Up @@ -412,7 +414,7 @@ QUnit.module( "dump" );
QUnit.test( "dump output", function( assert ) {
assert.equal( QUnit.dump.parse( [ 1, 2 ] ), "[\n 1,\n 2\n]" );
assert.equal( QUnit.dump.parse( { top: 5, left: 0 } ), "{\n \"left\": 0,\n \"top\": 5\n}" );
if ( typeof document !== "undefined" && document.getElementById( "qunit-header" ) ) {
if ( document && document.getElementById( "qunit-header" ) ) {
assert.equal(
QUnit.dump.parse( document.getElementById( "qunit-header" ) ),
"<h1 id=\"qunit-header\"></h1>"
Expand Down Expand Up @@ -703,7 +705,7 @@ QUnit.test( "throws", function( assert ) {
);
});

if ( typeof document !== "undefined" ) {
if ( document ) {

QUnit.module( "fixture" );

Expand Down Expand Up @@ -917,6 +919,4 @@ QUnit.test( "Circular reference - test reported by soniciq in #105", function( a
});

// Get a reference to the global object, like window in browsers
}( (function() {
return this;
}.call()) ));
}( typeof global === "object" && global || this ));