JUnit #103
JUnit #103
Conversation
|
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 markdownlint-cli/markdownlint.js Line 161 in e857b96 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. |
Based on how eslint generates these reports, just setting all times to 0
Add single passed test case if now lint errors found
|
Sorry, didn’t get to this again. Fully expect to tomorrow, thanks for your patience! |
There was a problem hiding this comment.
Thanks again for doing this! I’ve left a lot of comments, but many of them are pretty trivial. The one thing that’s missing before I’d consider this done is test cases. But if you don’t feel like adding that, just let me know. I am happy to accept these changes and add the tests myself. Please let me know if any of my feedback is unclear.
markdownlint.js
Outdated
| .option('-f, --fix', 'fix basic errors (does not work with STDIN)') | ||
| .option('-s, --stdin', 'read from STDIN (does not work with files)') | ||
| .option('-o, --output [outputFile]', 'write issues to file (no console)') | ||
| .option('-o, --output <outputFile>', 'write issues to file (no console)') | ||
| .option('-j, --junit [junitFile]', 'write issues to file in junit format as well as to the console') |
There was a problem hiding this comment.
Is “junit” the preferred capitalization?
There was a problem hiding this comment.
looks like JUnit is the correct name, but I'm thinking it makes sense to leave the option name and file name in the current case, and just change the descriptive text?
Implementing changes requested in PR review
|
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: Anything else you'd like to see tested with these changes? |
|
OK, played around with this. Decided to replicate the four Also writing the tests pointed out a few flaws (I love when that happens!) that I corrected in |
|
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. |
| @@ -63,6 +64,13 @@ Because this option makes changes to the input files, it is good to make a backu | |||
|
|
|||
| > Because not all rules include fix information when reporting errors, fixes may overlap, and not all errors are fixable, `--fix` will not usually address all errors. | |||
|
|
|||
| ### JUnit report | |||
|
|
|||
| If the `-j`/`--junit` option is specified, `markdownlint-cli` will output an XML report in JUnit format. | |||
There was a problem hiding this comment.
Add link to some info about “JUnit format”, maybe?
| If the `-j`/`--junit` option is specified, `markdownlint-cli` will output an XML report in JUnit format. | ||
| This report can be used in CI/CD environments to make the `markdownlint-cli` results available to your CI/CD tool. | ||
| If both `-j`/`--junit` and `-o`/`--output` are specified, `-o`/`--output` will take precedence. | ||
| Only one of these options should be specified. |
There was a problem hiding this comment.
Maybe remove this, I think your previous line is clear.
| @@ -67,7 +67,7 @@ function readConfiguration(args) { | |||
| markdownlint.readConfigSync(userConfigFile, configFileParsers); | |||
| config = require('deep-extend')(config, userConfig); | |||
| } catch (error) { | |||
| console.warn('Cannot read or parse config file ' + userConfigFile + ': ' + error.message); | |||
| console.warn(`Cannot read or parse config file ${userConfigFile}: ${error.message}`); | |||
There was a problem hiding this comment.
Thanks for doing the others!
| }); | ||
| } else { | ||
| const className = program.stdin ? 'stdin' : program.args; | ||
| testSuite.testCase().className(className).name('markdownlint').time(0); |
There was a problem hiding this comment.
Maybe make this multi-line for consistency with above?
| .option('-c, --config <configFile>', 'configuration file (JSON, JSONC, JS, or YAML)') | ||
| .option('-i, --ignore <file|directory|glob>', 'file(s) to ignore/exclude', concatArray, []) | ||
| .option('-p, --ignore-path <file>', 'path to file with ignore pattern(s)') | ||
| .option('-r, --rules <file|directory|glob|package>', 'custom rule files', concatArray, []); |
There was a problem hiding this comment.
Just noticed the extra space here, mind removing it?
| const parsedXml = xmlParser.xml2js(xml, {compact: true}); | ||
| t.is(result.stdout, ''); | ||
| t.is(result.stderr, ''); | ||
| t.is(Object.keys(parsedXml.testsuites).length, 1); |
There was a problem hiding this comment.
See my separate comment about maybe just comparing the output text directly. What you have here is more correct and thorough, but it’s a lot more typing and harder to follow. I’m fine with either choice - up to you if you want to type more. :)
| fs.unlinkSync(junit); | ||
| }); | ||
|
|
||
| // WIP |
There was a problem hiding this comment.
Your test cases make sense to me!
|
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. :) |
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
lintResultobject 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-buildercode to see what all options that could be used, and discovered failures could have a "type" in addition to a "message" so I added thenameas 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.