Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cli): Adding JUnit test reports #19747

Merged
merged 11 commits into from
Jul 26, 2023
Merged

feat(cli): Adding JUnit test reports #19747

merged 11 commits into from
Jul 26, 2023

Conversation

skycoop
Copy link
Contributor

@skycoop skycoop commented Jul 6, 2023

This PR makes the following changes

  • Created a CompoundTestReporter to allow us to use multiple reporters
  • Implements JUnitTestReporter which writes JUnit XML to a path
  • Added a CLI flag/option --junit that enables JUnit reporting. By default this writes the report to stdout (and disables pretty reporting). If a path is provided, it will write the JUnit report to that file while the pretty reporter writes to stdout like normal

Output of deno -- test --allow-all --unstable --location=http://js-unit-tests/foo/bar --junit cli/tests/unit/testing_test.ts

<?xml version="1.0" encoding="UTF-8"?>
<testsuites name="deno test" tests="7" failures="0" errors="0" time="0.176">
    <testsuite name="file:///Users/cooper/deno/deno/cli/tests/unit/testing_test.ts" tests="7" disabled="0" errors="0" failures="0">
        <testcase name="testWrongOverloads" time="0.012">
        </testcase>
        <testcase name="nameOfTestCaseCantBeEmpty" time="0.009">
        </testcase>
        <testcase name="invalidStepArguments" time="0.008">
        </testcase>
        <testcase name="nameOnTextContext" time="0.029">
            <properties>
                <property name="step[passed]" value="step ... nested step"/>
                <property name="step[passed]" value="step"/>
            </properties>
        </testcase>
        <testcase name="originOnTextContext" time="0.030">
            <properties>
                <property name="step[passed]" value="step ... nested step"/>
                <property name="step[passed]" value="step"/>
            </properties>
        </testcase>
        <testcase name="parentOnTextContext" time="0.030">
            <properties>
                <property name="step[passed]" value="step ... nested step"/>
                <property name="step[passed]" value="step"/>
            </properties>
        </testcase>
        <testcase name="explicit undefined for boolean options" time="0.009">
        </testcase>
    </testsuite>
</testsuites>

@CLAassistant
Copy link

CLAassistant commented Jul 6, 2023

CLA assistant check
All committers have signed the CLA.

Cargo.toml Outdated Show resolved Hide resolved
cli/tools/test.rs Outdated Show resolved Hide resolved
skycoop added a commit that referenced this pull request Jul 10, 2023
 This PR breaks the addition of the `TestReporter` trait and refactoring
of `PrettyTestReporter` out of #19747. The goal is to enable the
addition of test reporters, including machine readable output.
cli/tools/test.rs Outdated Show resolved Hide resolved
cli/tools/test.rs Outdated Show resolved Hide resolved
cli/tools/test.rs Outdated Show resolved Hide resolved
cli/util/time.rs Outdated Show resolved Hide resolved
@skycoop skycoop marked this pull request as ready for review July 10, 2023 22:42
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me!

cli/tools/test.rs Outdated Show resolved Hide resolved
cli/tools/test.rs Outdated Show resolved Hide resolved
cli/util/time.rs Outdated Show resolved Hide resolved
cli/tools/test.rs Outdated Show resolved Hide resolved
cli/tools/test.rs Outdated Show resolved Hide resolved
@dsherret dsherret added this to the 1.36 milestone Jul 11, 2023
cli/args/flags.rs Outdated Show resolved Hide resolved
cli/tools/test.rs Outdated Show resolved Hide resolved
cli/tools/test.rs Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member

@skycoop I did this:

$ cargo build
$ ./target/debug/deno test -A --unstable --junit=./report.xml cli/tests/unit/http_test.ts

Then I used xunit-viewer like so:

$ xunit-viewer -r ./report.xml
Written to: /Users/ib/dev/deno/index.html

Then I opened the file in the browser and got this:

Screenshot 2023-07-14 at 02 27 21

Looks great, test steps are reported too - although in a bit quirky way... here's how it looks if there's a failing step:

Screenshot 2023-07-14 at 02 28 44

I did a quick comparison with what mocha is doing on a test file like so:

var assert = require('assert');
describe('Array2', function () {
  describe('#indexOf()', function () {
    it('should return -1 when the value is not present', function () {
      assert.equal([1, 2, 3].indexOf(4), -1);
    });

    it('should return -1 when the value is not present', function () {
        assert.equal([1, 2, 3].indexOf(2), 1);
      });

    for (var i = 0; i < 10; i++) {
      it(`generated ${i}`, function () {
        assert(false);
      });
    }
  });
});

And the report I got is:
Screenshot 2023-07-14 at 02 30 48

It appears mocha is flattening all deeply nested cases to top-level cases.

Do you feel it would be possible for us to do the same for our test steps? (Seems like the exact same problem we'd have to face if we decided to do #14390, so might be worth to address it now).

@skycoop
Copy link
Contributor Author

skycoop commented Jul 17, 2023

I think the representation is being unpacked that way since xunit-viewer casting the repeated property to an array. Since steps are non-standard, I think this will be represented a lot of different ways by a lot of different systems.

The issue comparing with mocha is that our internal representation is different. describe is not a test case, it's a test suite, hence why everything ends up nested under a suite called #indexOf(). Deno.test is not a test suite, it's a test case that can have other test cases nested into it, so we're in a really uncomfortable situation. If we un-nest every step as it's own test-case, we'll start double counting everything which will cause deceptive execution times and failure counts.

bartlomieju added a commit that referenced this pull request Jul 26, 2023
Pulled from #19747

Authored-by: Cooper Benson <skycoop@gmail.com>
mmastrac pushed a commit that referenced this pull request Jul 26, 2023
Pulled from #19747

Authored-by: Cooper Benson <skycoop@gmail.com>
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bartlomieju bartlomieju merged commit 0e4d6d4 into main Jul 26, 2023
10 checks passed
@bartlomieju bartlomieju deleted the skycoop/test-reporters branch July 26, 2023 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants