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

Add speed in -R json option (#4226) #4434

Merged
merged 8 commits into from
Sep 13, 2020
Merged

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
Contributor

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

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

test/reporters/json.spec.js Outdated Show resolved Hide resolved
test/reporters/json.spec.js Outdated Show resolved Hide resolved
test/reporters/json.spec.js Outdated Show resolved Hide resolved
wwhurin and others added 4 commits September 10, 2020 19:31
Co-authored-by: Christopher Hiller <boneskull@boneskull.com>
Co-authored-by: Christopher Hiller <boneskull@boneskull.com>
Co-authored-by: Christopher Hiller <boneskull@boneskull.com>
@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