Lax qunitjs version from "1.10.0" to "^1.10.0" #118

Closed
wants to merge 1 commit into
from

Conversation

5 participants
@Krinkle
Member

Krinkle commented Mar 13, 2015

Fixes #117

@kof

This comment has been minimized.

Show comment
Hide comment
@kof

kof Mar 13, 2015

Contributor

hmm why does it breaks travis?

Contributor

kof commented Mar 13, 2015

hmm why does it breaks travis?

@Krinkle

This comment has been minimized.

Show comment
Hide comment
@Krinkle

Krinkle Mar 13, 2015

Member

I can reproduce the error locally.

  throw new assert.AssertionError({
        ^
AssertionError: no errors
    at /home/travis/build/kof/node-qunit/test/testrunner.js:49:11
    at /home/travis/build/kof/node-qunit/lib/testrunner.js:182:24

It seems the assert library is a bit useless when it comes to error reporting. Adding the following exposes the actual error.

diff --git a/test/testrunner.js b/test/testrunner.js
index fdb3eb0..221f9f7 100644
--- a/test/testrunner.js
+++ b/test/testrunner.js
@@ -46,6 +46,9 @@ chain.add('base testrunner', function() {
             failed: 2,
             passed: 5
         };
+        if (err) {
+            throw err;
+        }
         a.equal(err, null, 'no errors');
         a.ok(res.runtime > 0, 'Date was modified');
         delete res.runtime;
Error: Called start() outside of a test context too many times
    at Object.extend.start (node-qunit/node_modules/qunitjs/qunit/qunit.js:277:11)
    at _require (node-qunit/lib/child.js:66:11)
    at node-qunit/lib/child.js:177:5
    at Array.forEach (native)
    at Object.<anonymous> (node-qunit/lib/child.js:176:15)

The actual test in question:

test('myAsyncMethod test', function() {
    ok(true, 'myAsyncMethod started');

    stop();
    expect(3);

    myAsyncMethod(function(data) {
        equal(data, 123, 'myAsyncMethod returns right result');
        equal(data, 321, 'this should trigger an error');
        start();
    });
});
Member

Krinkle commented Mar 13, 2015

I can reproduce the error locally.

  throw new assert.AssertionError({
        ^
AssertionError: no errors
    at /home/travis/build/kof/node-qunit/test/testrunner.js:49:11
    at /home/travis/build/kof/node-qunit/lib/testrunner.js:182:24

It seems the assert library is a bit useless when it comes to error reporting. Adding the following exposes the actual error.

diff --git a/test/testrunner.js b/test/testrunner.js
index fdb3eb0..221f9f7 100644
--- a/test/testrunner.js
+++ b/test/testrunner.js
@@ -46,6 +46,9 @@ chain.add('base testrunner', function() {
             failed: 2,
             passed: 5
         };
+        if (err) {
+            throw err;
+        }
         a.equal(err, null, 'no errors');
         a.ok(res.runtime > 0, 'Date was modified');
         delete res.runtime;
Error: Called start() outside of a test context too many times
    at Object.extend.start (node-qunit/node_modules/qunitjs/qunit/qunit.js:277:11)
    at _require (node-qunit/lib/child.js:66:11)
    at node-qunit/lib/child.js:177:5
    at Array.forEach (native)
    at Object.<anonymous> (node-qunit/lib/child.js:176:15)

The actual test in question:

test('myAsyncMethod test', function() {
    ok(true, 'myAsyncMethod started');

    stop();
    expect(3);

    myAsyncMethod(function(data) {
        equal(data, 123, 'myAsyncMethod returns right result');
        equal(data, 321, 'this should trigger an error');
        start();
    });
});
@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Mar 13, 2015

Member

The stacktrace points at: https://github.com/kof/node-qunit/blob/master/lib/child.js#L66

That _require() method is called multiple times, so this looks like a bug in node-qunit that newer QUnit versions help to uncover.

Member

jzaefferer commented Mar 13, 2015

The stacktrace points at: https://github.com/kof/node-qunit/blob/master/lib/child.js#L66

That _require() method is called multiple times, so this looks like a bug in node-qunit that newer QUnit versions help to uncover.

@kof

This comment has been minimized.

Show comment
Hide comment
@kof

kof Mar 13, 2015

Contributor

yeah QUnit.start(); is called multiple times, maybe in the older version it wasn't an issue ...

Contributor

kof commented Mar 13, 2015

yeah QUnit.start(); is called multiple times, maybe in the older version it wasn't an issue ...

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Mar 14, 2015

Member

FYI, this behavior "tightening" arrived in v1.16.0. The previous behavior was considered a bug, so we did not honor its backward compatibility.

Discussion: qunitjs/qunit#653 (comment)

Member

JamesMGreene commented Mar 14, 2015

FYI, this behavior "tightening" arrived in v1.16.0. The previous behavior was considered a bug, so we did not honor its backward compatibility.

Discussion: qunitjs/qunit#653 (comment)

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Mar 14, 2015

Member

Which hopefully implies that the bug fix needed here will be backwards compatible.

Member

jzaefferer commented Mar 14, 2015

Which hopefully implies that the bug fix needed here will be backwards compatible.

@nikolas

This comment has been minimized.

Show comment
Hide comment
@nikolas

nikolas Apr 10, 2015

Contributor

I ran into problems just trying to update node-qunit to QUnit v1.11.0: #110

Contributor

nikolas commented Apr 10, 2015

I ran into problems just trying to update node-qunit to QUnit v1.11.0: #110

@Krinkle

This comment has been minimized.

Show comment
Hide comment
@Krinkle

Krinkle Oct 28, 2015

Member

Please fix this. Users are unable to use node-qunit with newer test suites because APIs introduced in the last 2 years of QUnit development (e.g. assert.expect()) fail on node-qunit which is pinned to QUnit 1.10.0 still where that method didn't exist.

Member

Krinkle commented Oct 28, 2015

Please fix this. Users are unable to use node-qunit with newer test suites because APIs introduced in the last 2 years of QUnit development (e.g. assert.expect()) fail on node-qunit which is pinned to QUnit 1.10.0 still where that method didn't exist.

@kof

This comment has been minimized.

Show comment
Hide comment
@kof

kof Oct 28, 2015

Contributor

@Krinkle totally forgot this one, can we already update to an even newer version?

Contributor

kof commented Oct 28, 2015

@Krinkle totally forgot this one, can we already update to an even newer version?

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 28, 2015

Member

1.20.0 was just released, but that shouldn't really matter, since the ^ includes all versions below 2.0.0.

Member

jzaefferer commented Oct 28, 2015

1.20.0 was just released, but that shouldn't really matter, since the ^ includes all versions below 2.0.0.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 28, 2015

Member

That said, you should definitely test with 1.20.0, since it might uncover other bugs in node-qunit. For example, calling start() with a non-numeric argument will cause tests to fail (somewhat similar to the other issue discussed above).

Member

jzaefferer commented Oct 28, 2015

That said, you should definitely test with 1.20.0, since it might uncover other bugs in node-qunit. For example, calling start() with a non-numeric argument will cause tests to fail (somewhat similar to the other issue discussed above).

@kof

This comment has been minimized.

Show comment
Hide comment
@kof

kof Oct 28, 2015

Contributor

@Krinkle would you like to become a collaborator in this project?

Contributor

kof commented Oct 28, 2015

@Krinkle would you like to become a collaborator in this project?

@kof

This comment has been minimized.

Show comment
Hide comment
@kof

kof Oct 28, 2015

Contributor

also @jzaefferer ?

Contributor

kof commented Oct 28, 2015

also @jzaefferer ?

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 28, 2015

Member

Sure, also @leobalter, since he's the (new) QUnit project lead.

Member

jzaefferer commented Oct 28, 2015

Sure, also @leobalter, since he's the (new) QUnit project lead.

@kof

This comment has been minimized.

Show comment
Hide comment
@kof

kof Oct 28, 2015

Contributor

done

Contributor

kof commented Oct 28, 2015

done

@kof

This comment has been minimized.

Show comment
Hide comment
@kof

kof Oct 28, 2015

Contributor

welcome in team @jzaefferer @Krinkle @leobalter

Contributor

kof commented Oct 28, 2015

welcome in team @jzaefferer @Krinkle @leobalter

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 28, 2015

Member

@kof thanks. So, are you going to put some time into fixing this or other issues, or are you hoping for one of us to take over?

Member

jzaefferer commented Oct 28, 2015

@kof thanks. So, are you going to put some time into fixing this or other issues, or are you hoping for one of us to take over?

@kof

This comment has been minimized.

Show comment
Hide comment
@kof

kof Oct 28, 2015

Contributor

@jzaefferer I would be glad to get some help on this

Contributor

kof commented Oct 28, 2015

@jzaefferer I would be glad to get some help on this

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 28, 2015

Member

Okay. I've got some things to discuss with @leobalter anyway, maybe we can also do a triage of issues here.

Member

jzaefferer commented Oct 28, 2015

Okay. I've got some things to discuss with @leobalter anyway, maybe we can also do a triage of issues here.

@Krinkle

This comment has been minimized.

Show comment
Hide comment
@Krinkle

Krinkle Oct 28, 2015

Member

Thanks @kof!

Member

Krinkle commented Oct 28, 2015

Thanks @kof!

@Krinkle Krinkle closed this Mar 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment