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.

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

Bug 632572 - CommonJS test adapter #93

Merged
10 commits merged into from Feb 16, 2011

Conversation

Projects
None yet
2 participants
@Gozala
Copy link
Member

commented Jan 15, 2011

No description provided.

@mykmelez

This comment has been minimized.

Copy link

commented on packages/api-utils/lib/test/assert.js in eca6897 Feb 7, 2011

If I read this constructor code correctly, it manually creates an object assertionError whose prototype is AssertionError.prototype and then configures it with the options argument, but then it never returns it.

Meanwhile, if the constructor is called with the new keyword, as demonstrated in the example, then the constructor automatically creates an object this whose prototype is also AssertionError.prototype, but the code never configures it.

The code should either configure the automatically-created object this (in which case assertionError is not needed); or, if you want to make it possible to create an AssertionError by calling the constructor without the new keyword, it should return assertionError.

@mykmelez

This comment has been minimized.

Copy link

commented on packages/api-utils/lib/test/assert.js in eca6897 Feb 7, 2011

Erm, I think you mean ==, no?

@mykmelez

This comment has been minimized.

Copy link

commented on packages/api-utils/lib/test/assert.js in eca6897 Feb 7, 2011

These examples would be better with actual values, f.e.:

assert.equal(1, 1, "one is one");
assert.notEqual(1, 2, "one is not two");
@mykmelez

This comment has been minimized.

Copy link

commented on packages/api-utils/lib/test/assert.js in eca6897 Feb 7, 2011

These'd be easier to read (and thus potentially less risky to modify later by someone unfamiliar with the code) if the operator in the conditional that does the actual checking of the values was the one associated with the test function, i.e.:

/**
 * The equality assertion tests shallow, coercive equality with `==`.
 * @example
 *    assert.equal(1, 1, "one is one")
 */
equal: function equal(actual, expected, message) {
  if (actual == expected) {
    this.pass(message);
  }
  else {
    this.fail({
     actual: actual,
     expected: expected,
     message: message,
     operator: '=='
    });
  }
},
/**
 * The non-equality assertion tests for whether two objects are not equal
 * with `!=`.
 * @example
 *    assert.notEqual(1, 2, "one is not two")
 */
notEqual: function notEqual(actual, expected, message) {
  if (actual != expected) {
    this.pass(message);
  }
  else {
    this.fail({
      actual: actual,
      expected: expected,
      message: message,
      operator: '!=',
    });
  }
},
@mykmelez

This comment has been minimized.

Copy link

commented on packages/api-utils/lib/test/assert.js in eca6897 Feb 7, 2011

It would be useful to note that this function (and notDeepEqual) test for strict equality.

@mykmelez

This comment has been minimized.

Copy link

commented on packages/api-utils/lib/test/assert.js in eca6897 Feb 7, 2011

It looks like the description of the function got mushed together with the example here.

Also, this function comment and the one for throws use a different comment style than elsewhere in the document.

@mykmelez

This comment has been minimized.

Copy link

commented on packages/api-utils/lib/test/assert.js in eca6897 Feb 7, 2011

Any reason not to call the function throws? raises makes sense to someone who knows Python and some other languages, but given that the property name is throws, the function might as well be called the same thing, which makes it easier to find when tracking it down from a reference to it in a stacktrace.

@mykmelez

This comment has been minimized.

Copy link

commented on packages/api-utils/lib/test/assert.js in eca6897 Feb 7, 2011

This could use a comment. Are there two possible call signatures for this function, one that includes an Error argument and one in which that argument is omitted?

@mykmelez

This comment has been minimized.

Copy link

commented on packages/api-utils/lib/test/assert.js in eca6897 Feb 7, 2011

Your other internal functions are all prefixed with an underscore. Any reason not to do so here as well?

@mykmelez

This comment has been minimized.

Copy link

commented on packages/api-utils/lib/test.js in eca6897 Feb 7, 2011

Shouldn't this be called something like commonjs-test-adapter.js? It's current name doesn't give any indication that it's related to the CommonJS test specification.

This comment has been minimized.

Copy link
Owner Author

replied Feb 8, 2011

No it's not since main idea is to allow running CommonJS tests without changes and the do following require('test').run(tests) if we'll use different name tests will have to change. As an alternative separate package 'commonjs' can be created where this adapter can be moved to.

@mykmelez

This comment has been minimized.

Copy link

commented on packages/api-utils/lib/test.js in eca6897 Feb 7, 2011

funcitons -> functions

Also, suit -> suite (and suits -> suites) in a couple of places above and below this line.

@mykmelez

This comment has been minimized.

Copy link

commented on packages/api-utils/lib/test.js in eca6897 Feb 7, 2011

then -> than

@mykmelez

This comment has been minimized.

Copy link

commented on packages/api-utils/lib/test.js in eca6897 Feb 7, 2011

it's -> its

@mykmelez

This comment has been minimized.

Copy link

commented on eca6897 Feb 7, 2011

General code conventions also apply, including the ones I mentioned in the bug:

  • end statements with semi-colons (except inside one-statement one-line
    blocks);
  • don't cuddle elses;
  • separate items in multi-line lists with commas (and multi-line conditional
    expressions with logic operators) appended to the preceding lines rather than
    prepended to the following lines;
  • put the static operand on the right hand side of equality operations;
  • insert blank lines between major sections of code (including before
    comments);
if ('operator' in options)
assertionError.operator = options.operator;
assertionError.message = options.message;
assertionError.stack = new Error().stack;

This comment has been minimized.

Copy link
@Gozala

Gozala Feb 8, 2011

Author Member

You're right I had to return assertionError

}
},
/**
* The equality assertion tests shallow, coercive equality with `=`.

This comment has been minimized.

Copy link
@Gozala

Gozala Feb 8, 2011

Author Member

Thanks, I've fixed that!

* The non-equality assertion tests for whether two objects are not equal
* with `!=`.
* @example
* assert.notEqual(actual, expected, message_opt)

This comment has been minimized.

Copy link
@Gozala

Gozala Feb 8, 2011

Author Member

Ok I'll change examples

This comment has been minimized.

Copy link
@Gozala

Gozala Feb 8, 2011

Author Member

Also I inverted if clause to make it more readable.

}
// 11. Expected to throw an error:
// assert.throws(block, Error_opt, message_opt);
, throws: function raises(block, Error, message) {

This comment has been minimized.

Copy link
@Gozala

Gozala Feb 8, 2011

Author Member

Renaming function to throws. Also please note that reason to prefer throws over raises is compatibility with CommonJS.

@@ -0,0 +1,113 @@

This comment has been minimized.

Copy link
@Gozala

Gozala Feb 8, 2011

Author Member

Since main idea is to allow running CommonJS tests without changing them, we need to have preserve module id as well. According to the CommonJS test have this signature require('test').run(tests) if we'll use a different module id tests written for CommonJS will have to change, that violates whole purpose.

As an alternative I can suggest moving this into an independent package 'commonjs' where this and maybe other (in the future) CommonJS adapters can be moved to.

@Gozala

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2011

As I suggested on the IRC I have modified this pull request to point to a different bug.
https://bugzilla.mozilla.org/show_bug.cgi?id=632572

There for scope of is changed and it only aims to get CommonJS test adapter.

@Gozala

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2011

Also I think that code follows all the conventions, if you see where it's not can you please comment inline or post a line number ?

// See for details: http://wiki.commonjs.org/wiki/Unit_Testing/1.1
let Assert = suite.Assert || BaseAssert;

// Going through each item in the test suit and wrapping it into a

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 12, 2011

Contributor

Nit: suit -> suite

/**
* This function is a CommonJS test runner function, but since Jetpack test
* runner and test format is different from CommonJS this function shims given
* `exports` with all its tests into a Jetpack test format so that build in

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 12, 2011

Contributor

Nit: build in -> built-in

function AssertionError(options) {
let assertionError = Object.create(AssertionError.prototype);

if ("string" === typeof options)

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 12, 2011

Contributor

"string" === typeof options -> typeof options === "string"

Although in this case you could just do isString(options).

* must be contained by a message of the thrown exception, or a RegExp
* matching a message of the thrown exception.
* @param {String} message
* Description message

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 12, 2011

Contributor

None of the CommonJS Unit Testing specifications I found (1.0, B, 1.1) describe throws adequately, but they do suggest that both the Error and the message parameters are optional.

But that's a problem for the Error argument polymorphism attempted here, since it isn't possible to distinguish between throws(Function block, String Error) and throws(Function block, String message). This implementation assumes the latter, but the former is just as possible.

We should avoid this ambiguity by removing String as a possible type for the argument to the Error parameter. RegExp should be able to cover the String cases without too much trouble.

It does make me wonder, though... are the String and RegExp types part of the CommonJS standard or extensions to it?

This comment has been minimized.

Copy link
@Gozala

Gozala Feb 13, 2011

Author Member

Yes that's a fare comment, I guess I'll remove string option. Also as you mentioned I can't see String or RegExp types in specs but I remember discussing them on the mailing list. Let's keep just RegExp and I'll make sure to communicate that back to the CommonJS mailing list so that it will be fixed in the 1.2 / 1.1.1

"use strict";

/**
* Returns `true` if `value` is not an `undefined`.

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 12, 2011

Contributor

Erm, you mean is undefined, no?

exports.isArguments = isArguments;

/**
* Returns true if it is a primitive `value`. (null, undefined, number, boolean)

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 12, 2011

Contributor

string should be added to the list of primitive values.

* isAtom('foo') // true
* isAtom({ bar: 3 }) // false
*/
function isAtom(value) {

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 12, 2011

Contributor

This should be called isPrimitive, as primitive is the conventional descriptor for these values.

*/
function isFlat(object) {
return isObject(object) &&
isNull(Object.getPrototypeOf(Object.getPrototypeOf(object)));

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 12, 2011

Contributor

What happens when you call isFlat(Object.prototype)?

* Returns `true` if `value` is an array / flat object containing only atomic
* values and other flat objects.
*/
function isJSON(value, visited) {

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 12, 2011

Contributor

Why not just try to JSONify it and return false if that throws an exception?

This comment has been minimized.

Copy link
@Gozala

Gozala Feb 13, 2011

Author Member

The problem is that JSON.stringify({ method: function() {} }) === "{}" // true is not going to throw instead it will evaluate to true. This is also a reason to have such an utility function, since sometimes you want to make sure that value won't get broken if jsonified.

var isConstructorNameSame;
var isConstructorSourceSame;

// If `instanceof` returned `true` we know result right a way.

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 12, 2011

Contributor

a way -> away

// If `instanceof` returned `true` we know result right a way.
var isInstanceOf = value instanceof Type;

// If `instanceof` returned `false` we do ductype check since `Type` may be

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 12, 2011

Contributor

ductype -> ducktype

name = '"' + name + '"';

if (descriptor.writtable)
result += "writtable ";

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 12, 2011

Contributor

writtable -> writable

exports["test function"] = function (assert) {
assert.ok(utils.isFunction(function(){}), "value is function");
assert.ok(utils.isFunction(Object), "Object is function");
assert.ok(utils.isFunction(new Function("")), "Genertaed value is function");

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 12, 2011

Contributor

Genertaed -> generated

assert.ok(utils.isFlat({}), "`{}` is a plain object");
assert.ok(!utils.isFlat([]), "`[]` is not a plain object");
assert.ok(!utils.isFlat(new function() {}), "derived objects are not plain");
};

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 12, 2011

Contributor

Adding assert.ok(!utils.isFlat(Object.prototype)); here throws:

error: TEST FAILED: test-utils.test types.test flat objects (exception)
error: An exception occurred.
Traceback (most recent call last):
  File "resource://api-utils-api-utils-lib/timer.js", line 64, in notifyOnTimeout
    this._callback.apply(null, this._params);
  File "resource://api-utils-api-utils-lib/unit-test.js", line 223, in 
    timer.setTimeout(function() { onDone(self); }, 0);
  File "resource://api-utils-api-utils-lib/unit-test.js", line 248, in runNextTest
    self.start({test: test, onDone: runNextTest});
  File "resource://api-utils-api-utils-lib/unit-test.js", line 266, in start
    this.test.testFunction(this);
  File "resource://api-utils-api-utils-lib/unit-test-finder.js", line 57, in runTest
    test(runner);
  File "resource://api-utils-api-utils-lib/test.js", line 93, in 
    test(assert);
  File "resource://api-utils-api-utils-tests/utils/type.js", line 33, in 
    assert.ok(!utils.isFlat(Object.prototype));
  File "resource://api-utils-api-utils-lib/utils/type.js", line 167, in isFlat
    isNull(Object.getPrototypeOf(Object.getPrototypeOf(object)));
TypeError: Object.getPrototypeOf(object) is not an object
"json can not contain functions");

assert.ok(!utils.isJSON(Object.create({})),
"json must be direct decedant of `Object.prototype`");

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 12, 2011

Contributor

decedant -> descendant

Is that actually true, though? JSON.stringify(Object.create({}) works just fine.

This comment has been minimized.

Copy link
@Gozala

Gozala Feb 13, 2011

Author Member

Well JSON.stringify({ method: function() {}, a: 'b' }) works just fine as well but, the point is that some of the data will be lost. Also the main purpose of isJSON function is to make sure that JSON.parse(JSON.stringify(value)) will not loose anything from the given value.

@@ -0,0 +1,349 @@

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 12, 2011

Contributor

It seems like unnecessary complexity to put this file by itself inside a utils subdirectory, especially since the name of the package itself is api-utils. Let's leave this as a top-level file.

@Gozala

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2011

Not sure why this pull request does not wants to pick up new changes so I made a new one:

#108

@mykmelez

This comment has been minimized.

Copy link

commented on packages/api-utils/lib/test.js in a0b2ab1 Feb 15, 2011

You don't need to add this extra space at the beginnings of blocks.

@mykmelez

This comment has been minimized.

Copy link

commented on packages/api-utils/lib/test.js in 8c1fd62 Feb 15, 2011

build-in -> built-in (or, even more correctly, "the built-in")

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.