Skip to content

Conversation

@wwhurin
Copy link
Contributor

@wwhurin wwhurin commented Sep 1, 2020

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

Description of the Change

I modified the -R json option to show speed.

Alternate Designs

N/A

Why should this be in core?

#4226

Benefits

You can see speed result in json reporter

Possible Drawbacks

N/A

Applicable issues

Fixes #4226

@boneskull boneskull added type: feature enhancement proposal area: node.js command-line-or-Node.js-specific area: reporters involving a specific reporter labels Sep 2, 2020
Copy link
Member

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

Thanks! Please update test/reporters.json.spec.js to reflect this change. You would likely modify an existing assertion, e.g.,

      expect(runner, 'to satisfy', {
        testResults: {
          failures: [
            {
              title: testTitle,
              file: testFile,
              speed: expect.it('to be greater than', 0),
              err: {
                message: error.message
              }
            }
          ]
        }
      });

Copy link
Contributor

@outsideris outsideris left a comment

Choose a reason for hiding this comment

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

Could add this in lib/reporters/json-stream.js as well?
And it needs to change test/reporters/json-stream.spec.js like boneskull said.

@wwhurin
Copy link
Contributor Author

wwhurin commented Sep 6, 2020

Thanks! Please update test/reporters.json.spec.js to reflect this change. You would likely modify an existing assertion, e.g.,

      expect(runner, 'to satisfy', {
        testResults: {
          failures: [
            {
              title: testTitle,
              file: testFile,
              speed: expect.it('to be greater than', 0),
              err: {
                message: error.message
              }
            }
          ]
        }
      });

I have changed json.spec.js! However, speed is not a number, but things like 'fast' and'slow'. Also, speed does not pass unless it passes. But json.spec.js does not have a pass case, so I added a pass case to check the speed option. Would it be okay? 🤔

@wwhurin
Copy link
Contributor Author

wwhurin commented Sep 6, 2020

Could add this in lib/reporters/json-stream.js as well?
And it needs to change test/reporters/json-stream.spec.js like boneskull said.

OK. I changed json-stream.js and json-stream.spec.js! You can see the speed in json-stream option.

Copy link
Contributor

@outsideris outsideris left a comment

Choose a reason for hiding this comment

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

Thank you.

Copy link
Member

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

Thanks for updating this. I have a few suggestions. If you don't apply them, I will do so after merge.

@outsideris outsideris merged commit 738a575 into mochajs:master Sep 13, 2020
@boneskull boneskull added the semver-minor implementation requires increase of "minor" version number; "features" label Oct 7, 2020
@boneskull boneskull added this to the v8.2.0 milestone Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: node.js command-line-or-Node.js-specific area: reporters involving a specific reporter semver-minor implementation requires increase of "minor" version number; "features" type: feature enhancement proposal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose test.speed (or slow value) in reports (at least JSON)

3 participants