Fix qunit adapter

[flore77]

Fixes #57.

[flore77]

@jzaefferer I know the tests are still failing, but I want to have some feedback for this part.

There are still 2 big issues to solve: suiteEnd objects and nested suites.

Here is does not function the trick to add to the test object the errors property, because QUnit is emitting other objects on each event. So on the suiteEnd event, the emitted objects will contain a list of tests, but this have only 2 props: name and testId. We need also runtime, error and status.

I am thinking to index the aforementioned props into an array, something:

var testProps = {}

var props = {
  status: test.status,
  errors: test.errors,
  runtime: test.runtime
}

testProps[test.testId] = props. 

So in the convertTest function, if we don't have the all the props we need, we can check for them in the testProps object.

[jzaefferer]

So in the convertTest function, if we don't have the all the props we need, we can check for them in the testProps object.

Let's give that a try.

[flore77]

@jzaefferer please review.

[flore77]

Done.

[jzaefferer]

That looks better. npm test is failing for me with this output:

/Users/jzaefferer/dev/jquery/js-reporters/dist/js-reporters.js:1175
          if (actual.name !== expected.name) {
                    ^

TypeError: Cannot read property 'name' of undefined

Haven't tried to debug that, yet, are you aware of this issue?

[flore77]

Yes, I have seen it, but I still wanted to push, because the integration tests are passing, at least some of them that should pass, on behalf the fixes :laughing:

Haven't tried to debug that, yet

I will debug it!

Maybe it gets undefined, because now there are less events, we do not emit the globalSuite twice.

[flore77]

I found the problem and it is a bug :disappointed: The globalModule is not emitted if it does not contain any tests, so on the runEnd event this.globalSuiteEnd will be undefined. In our test fixture we have a global test so everything is functioning well.

EDIT: Also now on the runEnd event it is not emitted quite a well built suite, so we must still take care of emitting a good built suite and I think taking care of this should solve also the aforementioned problem, this is why is not quite a bug, but it is a good case to take in consideration, that our QUnit fixture does not cover.

[flore77]

I would leave this after we found a fix for the nested suites, because also now the suite on the runStart event is not created quite well. After fixing the nesting suites, it would be more clearer how it must be built.

[jzaefferer]

I didn't understand your explanations. Let's make npm test pass, even if that means skipping a few tests temporarily.

[flore77]

The onBegin function does not emit a suite that respects the standard, because:

• it does not treat nested suites, i.e the outter suite has no childSuites (it should have the inner suite), all suites are putted as childSuites of the globalSuite
• QUnit global module is wrapped in an another module instead of being the one that wraps all other modules

The moduleDone functions saves in this.globalEndSuite a converted suite that does not contain the other suites, something of this form:

Suite {
  name: '',
  childSuites: [],
  tests: 
   [ Test {
       testName: 'global test',
       suiteName: undefined,
       status: 'passed',
       runtime: 1,
       errors: [] } ] }

I was thinking to firstly fix the nested modules, and then to build the suites emitted on runStart and runEnd, because then we will have the big picture, while now we don't now how we will deal with the nested suites.

The undefined comes from, because the old test fixture does not have a global test and QUnit does not emit the globalModule anymore on the suiteStart and suiteEnd events. And so our adapter will never enter on the else, so this.globalEndSuite will stay undefined.

I hope I have explained it more clearly :smiley:

How should I proceed ? Should I know to take care of building the suites properly for the runStart and runEnd events ? Or should I deal with the nested modules ?

[jzaefferer]

The undefined comes from, because the old test fixture does not have a global test and QUnit does not emit the globalModule anymore on the suiteStart and suiteEnd events. And so our adapter will never enter on the else, so this.globalEndSuite will stay undefined.

Is there a simple fix or workaround for that, so that we can make these tests pass while working on other issues?

How should I proceed ? Should I know to take care of building the suites properly for the runStart and runEnd events ? Or should I deal with the nested modules ?

Either seems fine to me, I can't tell which one is better to start with. Pick one, if you get stuck, try the other?

[flore77]

@jzaefferer I have fixed the global suites emitted on the runStart/runEnd events, I am looking forward for your feedback :smiley:

The 2 tests that are testing this events are still failing, but because of the name of the inner suite.

QUnit emits also for suites a concatenated name, the inner suite has the name: outter suite > inner suiteand in our reference data it has the name inner suite. How should this be ?

The inner tests has also a concatenated name, but we must only remove the > character. I am waiting to know how we deal with the suite name, then I will solve them both.

[jzaefferer]

I'll have to review the createGlobalSuite() method more thoroughly later, at a glance it looks good. Nice job!

QUnit emits also for suites a concatenated name, the inner suite has the name: outter suite > inner suiteand in our reference data it has the name inner suite. How should this be ?

The inner tests has also a concatenated name, but we must only remove the > character. I am waiting to know how we deal with the suite name, then I will solve them both.

This feels like something we should bring back to QUnit. For now let's just cut of the last > and anything before that. That should be simple enough to adjust later.

[flore77]

Now remains only to solve the execution of nested modules.

I have seen that also Jasmine executes the nested suites this way, from the inner most to the outer most suite.

How should we do this in our standard ?

Also from what I have seen, QUnit does not emit the start of the outter suite and then the start of the inner suite, it executes them indepedently, i.e inner suite starts, inner test executes, inner suite ends, outer suite starts, outer test executes, outer suite ends, so any option we choose, we will have some code to write for the QUnitAdapter.

[jzaefferer]

Are the Jasmine and QUnit behaviours at least consistent? Or do they also have differences?

so any option we choose, we will have some code to write for the QUnitAdapter.

I have no preference, what do you think?

[flore77]

Are the Jasmine and QUnit behaviours at least consistent? Or do they also have differences?

Yes, there are some small differences, but I didn't investigate too deep for Jasmine.

I think the most natural is to run the tests from the outer most suite to the inner most suite, like Mocha does. Also the QUnit reporter displays tests in the same order as Mocha, no matter in which order the tests are executed and emitted.

So, I think definitely to emit the tests from the outer most to the inner most.

I will try today to achieve this for QUnit, I am thinking to buffer all data in our adapter until QUnits calls the done callback and after that to begin emit data from our adapter. This way we can store and order easily the data in a form we like. Sounds ok ?

[jzaefferer]

Yes, that sounds good. Could you also file an issue against QUnit, telling them about the difference with Mocha, and the inconsistency in the UI? Maybe they're interested in addressing that before the 2.0.0 release.

[jzaefferer]

Before you work on the suites handling, could we skip those 8 failing tests and land this PR? Then work on the suites handling in a separate PR.

[flore77]

Could you also file an issue against QUnit, telling them about the ...

Hmm, I don't know, QUnit has 2 options that randomizes tests execution:

• the reorder option that on a rerun, runs firstly the failed tests, it is activated by default
• the seed option that randomizes tests execution, it is disabled by default

The reorder option is activated by default and in it description it says:

this can speed up testing a lot. It can also lead to random errors, in case your testsuite has non-atomic tests, where the order is important. You should fix those issues, instead of disabling reordering!`

So, I think it is an important feature of QUnit and I think in their reporter they are doing this reordering, because it would be confusing on multiple runs to display tests in a different order.

Of course the inconsistency is that on a normal running they are running the tests from the inner most to the outer most, but displaying them the other way, but I think the reporter cannot be change, because it must handle the other cases of reorder and seed. So the only change that it can be done, would be to change the normal running, but I don't know if it would be a gain.

What do you think? Should I open an issue there to see what they say about it ?

[jzaefferer]

Thanks for the details, I hadn't considered those. So no, let's not do that now.

As for this PR, let's skip the remaining failing tests and merge.

[flore77]

Yeah, we have a problem, the rollup-plugin-babel is causing this:

Cannot find module 'babelHelpers' from '/home/flore77/js-reporters/node_modules/events', I don't know yet how to solve this, I have forced to stay for now at the 2.4.0.

[flore77]

Maybe we should change the assertion of the runtime range, to check only if runtime is a number, like:

expect(typeof runtime).to.be.equal('number').

[jzaefferer]

Maybe we should change the assertion of the runtime range

Let's see if the updated assertion works out, if not, we could still make that change.