search

qunitjs / js-reporters

📋 Common Reporter Interface (CRI) for JavaScript testing frameworks.
MIT License
60 stars 18 forks source link

Adds mocha-adapter #36

Closed nikhilshagri closed 8 years ago

nikhilshagri commented 8 years ago

Finishes up the PR by @fcarstens from #28

nikhilshagri commented 8 years ago

@jzaefferer I forgot to npm run test before pushing changes. Will fix them shortly.

jzaefferer commented 8 years ago

There's also something weird about your commit. When looking at the commit by itself, it has the same diff as this PR. Not sure how that happened, but it makes the effort of commit previous commits rather usless. Specifically, I can't tell what you changed, if anything. Can you please look into that, along with fixing tests? Thanks.

jzaefferer commented 8 years ago

PS: Please comment here when you push more commits (there are no notifications for pushes/new commits).

nikhilshagri commented 8 years ago

Not sure why it's happening, maybe because the commit is a merge commit, and therefore it shows the diff of the two parent commits: https://github.com/blog/559-merge-commits-are-back-and-better-than-ever

I think I only made one change, which was deleting line #92 in lib/MochaAdapter.js, the one which threw the testStart event for pending tests. I left the mocha.setup('bdd') as it was, because it was necessary, and I forgot to change the xit into it.skip.

Also, the index.html page in test/mocha did not show any output when run on the web browser. I will try to fix that too and then push commits here.

jzaefferer commented 8 years ago

You shouldn't need a merge commit when doing a rebase. If you only changed one line, try resetting to the original branch (git reset --hard [name of branch]), then doing the rebase (git rebase master), then adding new commit with your changes.

nikhilshagri commented 8 years ago

I reset my master and rebased, which turned out fine. Then, I tried to apply my change on it(deleting line #92) and then did npm run test. This results in some pretty weird behaviour, where the expected and actual events are off by one( I really don't know how to explain it). I tried to find the reason for it, but nothing came up. I can post the error output to a gist if you want to take a look at it.

jzaefferer commented 8 years ago

I suggest you just push that state to this branch/PR, then I can try it out locally myself.

nikhilshagri commented 8 years ago

Done.

jzaefferer commented 8 years ago

This results in some pretty weird behaviour, where the expected and actual events are off by one( I really don't know how to explain it). I tried to find the reason for it, but nothing came up. I can post the error output to a gist if you want to take a look at it.

I just tried to reproduce this, but didn't get any errors. npm test finishes without any input, which should be okay here.

Can you gist what output you got?

PS: Landed a few commits in master, would be good to rebase this once more

nikhilshagri commented 8 years ago

Ah, seems like I have some competition here ;) Like I said, this happened when I tried to delete( or comment out) the this.emit("testStart", test); line in MochaAdapter.js. I could not push that change here or else the Travis build would have failed. Here's the gist: https://gist.github.com/cynicaldevil/d3aedde700156ea57589#file-output-js

jzaefferer commented 8 years ago

There's a lot of output, but it looks like its mostly a off-by-one issue, where everything after the first failure is also off. May need to adjust tests to deal with that line-removal. Or maybe the line is actually correct, just needs a comment explaining why its there.

nikhilshagri commented 8 years ago

I feel like that the line was needed, May have to do something with Mocha's internal working.

jzaefferer commented 8 years ago

We can improve the adapter later, if we missed something. Besides the questionable line, was there anything else we should address here? If not, I'll merge this.

nikhilshagri commented 8 years ago

There is a small issue: The HTML page which runs the tests on the browser does not display anything.

jzaefferer commented 8 years ago

Okay. Have you tried debugging that?

nikhilshagri commented 8 years ago

I have. I found that by commenting out the lines related to initializing the js-reporters runner object, test results of group a were displayed. Couldn't find anything else, but I could try fixing it if you want it.

jzaefferer commented 8 years ago

Yes please

nikhilshagri commented 8 years ago

The HTML page now shows all the results. But as a result of the js-reporters code being commented out, the test object is not displayed in the console. If it's ok with you, then I think the code's ready to be merged.

jzaefferer commented 8 years ago

Replaced by #46