Add browser log output to JUnit.xml #303
Conversation
|
Please, correct the commit msg https://docs.google.com/document/d/1JCFlkQ59Bx4qN1WKS1dtKXKl5G_lED6Lxey3iPNpQOs/pub |
|
Okay, I've got the commit message guidelines now. Fixed. |
|
Great. While we're at it could you please add an e2e test that verifies that this is working as it's supposed to? |
|
Good idea. Will do. |
|
This update is better, it has an e2e test which runs but I'm a little vague on how to go about verifying the output. Anyone able to point me in the right direction? |
| @@ -32,6 +32,7 @@ var createReporters = function(names, config, emitter) { | |||
| var multiReporter = new MultiReporter(); | |||
|
|
|||
| names.forEach(function(name) { | |||
| log.debug('Loading reporter "%s"...', name); | |||
There was a problem hiding this comment.
This is nice but not really related to what you are doing or do I miss something?
There was a problem hiding this comment.
This line is how I figured out that the e2e tests weren't actually loading the junit reporter.
|
I'm sorry, right now the e2e tests aren't configured to do output validation. It's the next thing on my todo list to setup a much better e2e testing environment. For now you can leave the e2e tests out, sorry for the confusion. |
|
Let's keep what I've got and roll it forward. I'm willing to do the work and at least now we verify that the reporter gets created. |
|
Yeah that's fine with me. But I think we should split the commit messages into one for the feature one for new test as the test. |
Make the JUnit adapter inherit from base Push all log message into system-out element of the junit xml file. Closes #302
E2E test framework was overriding the reporters on the commandline. This meant that tests e2e tests for reporter were not even using the reporters they were supposed testing. The test now use the specified reporters, even if they don't verify their outputs.
|
Done. Look good? |
| var xml; | ||
| var suites; | ||
| var pendingFileWritings = 0; | ||
| var fileWritingFinished = function() {}; | ||
|
|
||
| this.adapters = []; |
There was a problem hiding this comment.
You can't remove this line. testacular run would fail.
I know, the way it's currently implemented is not nice, but until we refactor it, we need to keep this.
(when you do testacular run, it appends another adapter, to basically stream the output to the runner https://github.com/testacular/testacular/blob/master/lib/server.js#L164 and https://github.com/testacular/testacular/blob/master/lib/reporters/Multi.js#L9)
There was a problem hiding this comment.
Btw, this is another example, that we just need better e2e tests, because weirdness like this is difficult to spot...
There was a problem hiding this comment.
Does my making this reporter inherit from BaseReporter change anything you just said about this line? If I kept this it would overwrite the adapter that was just added, wouldn't it?
There was a problem hiding this comment.
Man, you got me ;-) You are right!
|
Thanks man! Just bring the one line back and I'm gonna merge it. |
|
Actually, would you mind sending the pull request to https://github.com/testacular/testacular-junit-reporter instead ? The refactoring to support plugins should get merged soon and I already have all the stuff separated into plugins. I already merged your second commit as 87f0ac0 Let me know if you need any help. |
|
Okay, I'll apply this change to that repo. |
Made the JUnit adapter inherit from base and push all log message into an array for output to the xml file.