JUnit #103

FISHMANPET · 2020-06-25T16:42:53Z

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.

DavidAnson · 2020-06-25T16:53:59Z

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. :)

FISHMANPET · 2020-06-25T17:21:43Z

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

process.exitCode = 1;

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.

FISHMANPET · 2020-06-25T17:31:37Z

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

DavidAnson · 2020-06-27T06:33:54Z

Sorry, didn’t get to this again. Fully expect to tomorrow, thanks for your patience!

DavidAnson

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

DavidAnson · 2020-06-27T20:20:07Z

markdownlint.js

  .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')


Is “junit” the preferred capitalization?

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?

markdownlint.js

package.json

markdownlint.js

Implementing changes requested in PR review

FISHMANPET · 2020-06-29T19:04:54Z

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?

FISHMANPET · 2020-06-29T23:00:45Z

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.

DavidAnson · 2020-06-29T23:05:48Z

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.

DavidAnson

I’m loving it! Thanks very much for following up on the feedback. We’re getting very close now, just a need to round out the test story.

DavidAnson · 2020-06-30T06:17:21Z

README.md

@@ -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.


Add link to some info about “JUnit format”, maybe?

DavidAnson · 2020-06-30T06:18:10Z

README.md

+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.


Maybe remove this, I think your previous line is clear.

DavidAnson · 2020-06-30T06:18:39Z

markdownlint.js

@@ -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}`);


Thanks for doing the others!

DavidAnson · 2020-06-30T06:20:41Z

markdownlint.js

+      });
+    } else {
+      const className = program.stdin ? 'stdin' : program.args;
+      testSuite.testCase().className(className).name('markdownlint').time(0);


Maybe make this multi-line for consistency with above?

DavidAnson · 2020-06-30T06:22:47Z

markdownlint.js

+  .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, []);


Just noticed the extra space here, mind removing it?

DavidAnson · 2020-06-30T06:26:15Z

test/test.js

+  const parsedXml = xmlParser.xml2js(xml, {compact: true});
+  t.is(result.stdout, '');
+  t.is(result.stderr, '');
+  t.is(Object.keys(parsedXml.testsuites).length, 1);


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. :)

DavidAnson · 2020-06-30T06:26:35Z

test/test.js

+  fs.unlinkSync(junit);
+});
+
+// WIP


Your test cases make sense to me!

FISHMANPET · 2021-06-30T14:09:15Z

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.

DavidAnson · 2021-06-30T22:23:34Z

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. :)

FISHMANPET added 3 commits June 25, 2020 00:24

add junit

1ac8cd3

Update markdownlint.js

95cb1ff

Breakup long lines

170f6a7

FISHMANPET added 2 commits June 25, 2020 12:35

Setting time to 0

60d6870

Based on how eslint generates these reports, just setting all times to 0

Update markdownlint.js

30aa9cf

Add single passed test case if now lint errors found

DavidAnson requested changes Jun 27, 2020

View reviewed changes

PR Change requests

c4e3f57

Implementing changes requested in PR review

start implementing tests

4be73c0

FISHMANPET changed the title ~~Junit~~ JUnit Jun 29, 2020

Updated test name and internal variables

88e95c1

DavidAnson requested changes Jun 30, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JUnit #103

JUnit #103

FISHMANPET commented Jun 25, 2020

DavidAnson commented Jun 25, 2020

FISHMANPET commented Jun 25, 2020

FISHMANPET commented Jun 25, 2020

DavidAnson commented Jun 27, 2020

DavidAnson left a comment

DavidAnson Jun 27, 2020

FISHMANPET Jun 29, 2020

DavidAnson Jun 30, 2020

FISHMANPET commented Jun 29, 2020

FISHMANPET commented Jun 29, 2020

DavidAnson commented Jun 29, 2020

DavidAnson left a comment

DavidAnson Jun 30, 2020

DavidAnson Jun 30, 2020

DavidAnson Jun 30, 2020

DavidAnson Jun 30, 2020

DavidAnson Jun 30, 2020

DavidAnson Jun 30, 2020

DavidAnson Jun 30, 2020

FISHMANPET commented Jun 30, 2021 •

edited

Loading

DavidAnson commented Jun 30, 2021

JUnit #103

Are you sure you want to change the base?

JUnit #103

Conversation

FISHMANPET commented Jun 25, 2020

DavidAnson commented Jun 25, 2020

FISHMANPET commented Jun 25, 2020

FISHMANPET commented Jun 25, 2020

DavidAnson commented Jun 27, 2020

DavidAnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FISHMANPET commented Jun 29, 2020

FISHMANPET commented Jun 29, 2020

DavidAnson commented Jun 29, 2020

DavidAnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FISHMANPET commented Jun 30, 2021 • edited Loading

DavidAnson commented Jun 30, 2021

FISHMANPET commented Jun 30, 2021 •

edited

Loading