igorshubovych
JUnit
Fixes #58 As requested, a PR for my changes. I added a little bit since I wrote the comment last night, in 95cb1ff.
I added some time and timestamps because Azure Devops was complaining about not having them, and while Azure Devops could handle it I suspect other CIs might not. Since there are no time or duration values in the lintResult object that was already being constructed (and it doesn't look like the base markdownlint library produces any either) I just put in some simple defaults: The timestamp for the test suite will be the time the report is generated, the time for each testcase will just be 1 millisecond (spec is for time in seconds so 0.001 seconds), and the total time for the testcase will be the elapsed time between when the report generation started and when it finishes (which will only be a couple of milliseconds most likely).
I looked a bit at the junit-report-builder code to see what all options that could be used, and discovered failures could have a "type" in addition to a "message" so I added the name as the type for each failure.
I don't know that there's an official formal JUnit schema, but Azure Devops links to this one I'm not very good at reading xml schema documents but it looks like there's some fields that are required according to the schema but that this module doesn't support. I believe I've used every test suite and test case attribute that exists in the module, and based on its popularity I'm guessing what it produces is acceptable to most if not all CI tools, but if someone would ever report an issue about this generating something that their CI tool can't read, it would probably be an upstream issue for that module to support it before this project can support it.
I appreciated the pre-commit linter (once I realized it was linter errors that were preventing me from committing). I split up the test suite and test case across multiple lines since they were pretty long, I'm pretty sure there will be other style changes you'd like to see made.
And testing, if you point me in a direction I can try to implement some kind of testing.
Great, thanks, I’ll try to review this tonight! One thing you might do if bored is look at how other projects, especially ESLint, handle some of these things (like time stamp) in their jUnit output. Probably can’t go wrong being consistent. :)
Also it turns out this will still throw an exit code of 1 when there are test failures because of https://github.com/igorshubovych/markdownlint-cli/blob/e857b964a04c8d81f660f65951573d52294fcc82/markdownlint.js#L161
I thought about wrapping that in something like if (!program.junit) or adding a new parameter like --no-non-zero-exit-code-on-linting-errors but you said you wanted to consider that separately. I could open a new issue if you'd like to discuss that part.
Good idea to look at what eslint does. I'll need to grok what they're doing a little bit, but at the very least it makes me realize that I'm not sure what will happen if there are no markdown errors, I think I need to add an explicit case like eslint to report a single successful test case if there are no issues.
Sorry, didn’t get to this again. Fully expect to tomorrow, thanks for your patience!
I pushed changes for all the comments I resolved in c4e3f57. Looking at how tests are currently being done it shouldn't be too difficult to put some tests in.
Case/Situations I think should be tested:
JUnit report when there are issues (will pick one of your "incorrect" md test documents)
JUnit report when there aren't issues (will pick one of your "correct" md test documents)
When both -o and -j are specified, ensure that the -o section is what's run and the JUnit stuff will be skipped.
Anything else you'd like to see tested with these changes?
OK, played around with this. Decided to replicate the four --output tests with --junit and started with the first one, which was nothing in via stdin. I settled on using an XML parser to parse the output XML, settling with xml-js (added as a dev dependency to package.json). I figured I'd push the first test to see if you agree with the methodology before I did more. I'm not sure if subsequent tests would be quite so rigorous, as it is that test checked everything that the builder writes, but subsequent tests should only need to test the things that are different from the first test.
Also writing the tests pointed out a few flaws (I love when that happens!) that I corrected in markdownlint.js. Now if the input is stdin then the classname will be stdin instead of program.args. And writing an empty string to console.error is different from not writing to console.error, so I set it to only write to the console if there are actual results.
I haven’t had a chance to look at any of this yet, but saw this latest message fly by and wanted to chime in in case it could save you some time. What do you think about just doing a plain text comparison of the output instead of trying to re-parse it as XML? I don’t imagine the layout or format will change unexpectedly, so does this give us enough confidence (assuming that we make sure the text output is correct)? Or, if you’ve already done this, maybe it’s best to be general and I do like that there will be guaranteed validation of the XML structure with a parser.
As is the case with open source, I got distracted from this for a while and now (exactly a year later!) I'm looking at it again.
I believe at the time I got caught up on tests, and decided to implement an overly rigorous test regime (using the XML parser). You recommended just comparing to a known file and... I'm not sure why I ignored that at the time.
I also see you've adapted much of this into markdownlint-cli2 so I'm not sure there's any benefit to "finishing" this. If you'd like I could redo the tests to just match against a known file, and align the output with what you've done with the junit formatter in cli2. But I read your post about why you wrote cli2, and my guess is you'd be fine just closing this without merging, since the bulk of the work has been implemented there.
Glad to have you back! What do you do here is kind of up to you! I wrote CLI2 for the reasons you read about, but I don't expect everybody to migrate over immediately - or even ever. Having a compatible implementation for this project would be nice and probably benefit some people. On the other hand, not having it represents a small carrot for people to migrate. I'm happy to support whatever approach you choose. :)