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

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

Closed
wants to merge 1 commit into from

Conversation

Krinkle
Copy link
Member

@Krinkle Krinkle commented Mar 13, 2015

Fixes #117

@kof
Copy link
Contributor

kof commented Mar 13, 2015

hmm why does it breaks travis?

@Krinkle
Copy link
Member Author

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
Copy link
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.

@kof
Copy link
Contributor

kof commented Mar 13, 2015

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

@JamesMGreene
Copy link
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)

@jzaefferer
Copy link
Member

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

@nikolas
Copy link
Contributor

nikolas commented Apr 10, 2015

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

@Krinkle
Copy link
Member Author

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
Copy link
Contributor

kof commented Oct 28, 2015

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

@jzaefferer
Copy link
Member

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

@jzaefferer
Copy link
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).

@kof
Copy link
Contributor

kof commented Oct 28, 2015

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

@kof
Copy link
Contributor

kof commented Oct 28, 2015

also @jzaefferer ?

@jzaefferer
Copy link
Member

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

@kof
Copy link
Contributor

kof commented Oct 28, 2015

done

@kof
Copy link
Contributor

kof commented Oct 28, 2015

welcome in team @jzaefferer @Krinkle @leobalter

@jzaefferer
Copy link
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?

@kof
Copy link
Contributor

kof commented Oct 28, 2015

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

@jzaefferer
Copy link
Member

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

@Krinkle
Copy link
Member Author

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
Labels
None yet
5 participants