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

Include fullName in formattedAssertion #3378

Closed
wants to merge 1 commit into from

Conversation

aifreedom
Copy link
Contributor

@aifreedom aifreedom commented Apr 26, 2017

Summary
This PR adds an additional field fullName to FormattedAssertionResult.

When we try to analyze the test result json, we find that two it test cases under different describe block in one test file are rendered the same in the json report. Including the full name help solving this issue.

Test plan

The function I touched doesn't have a test. It was added here in #1988.

Sample json from my test run:

...
    "testResults": [
        {
            "assertionResults": [
                {
                    "failureMessages": [],
                    "fullName": "Header renders",
                    "status": "passed",
                    "title": "renders"
                },
                {
                    "failureMessages": [],
                    "fullName": "Header renders <Element /> with right props",
                    "status": "passed",
                    "title": "renders <Element /> with right props"
                }
            ],
            "endTime": 1493233733071,
            "message": "",
            "name": "/path/to/the/test/file.js",
            "startTime": 1493233730480,
            "status": "passed",
            "summary": ""
        }
    ],
...

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email [email protected] with your details so we can update your status.

@codecov-io
Copy link

Codecov Report

Merging #3378 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3378   +/-   ##
=======================================
  Coverage   64.89%   64.89%           
=======================================
  Files         176      176           
  Lines        6521     6521           
  Branches        4        4           
=======================================
  Hits         4232     4232           
  Misses       2288     2288           
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-util/src/formatTestResults.js 0% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfcc4b2...07915ed. Read the comment docs.

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@cpojer
Copy link
Member

cpojer commented Apr 28, 2017

We actually have a lint rule in eslint-plugin-jest that prevents you from having two tests with the same name. I recommend using that instead – will that work for you?

@cpojer cpojer closed this Apr 28, 2017
@ljharb
Copy link
Contributor

ljharb commented Apr 28, 2017

That seems kind of orthogonal to "including more information in the results" - also the whole point of "describe" is that the "it" can be generic, since the describe provides more context.

@thymikee
Copy link
Collaborator

Actually what's wrong with adding just one extra property?

@ljharb
Copy link
Contributor

ljharb commented Apr 28, 2017

(To clarify; I'm saying that requiring "it"s to be unique makes no sense and runs counter to the entire point of BDD-style test frameworks like jasmine, jest, and mocha)

@cpojer
Copy link
Member

cpojer commented Apr 28, 2017 via email

@ljharb
Copy link
Contributor

ljharb commented Apr 28, 2017

Ah, that makes much more sense. :-)

In that case the linter rule seems very useful; but it'd still be useful to include this additional context in the output. Could it be reopened?

@cpojer
Copy link
Member

cpojer commented Apr 28, 2017 via email

@ljharb
Copy link
Contributor

ljharb commented Apr 28, 2017

I don't think the PR can be updated unless it's opened, and when a collaborator closes it, I don't think anyone but a collaborator can reopen it :-) how to solve this chicken and egg problem? :-p

@cpojer
Copy link
Member

cpojer commented Apr 28, 2017

You just go and send a new PR.

@ljharb
Copy link
Contributor

ljharb commented Apr 28, 2017

and if my coworker (the OP) wants to write the tests instead, they should send a new PR?

@aifreedom aifreedom deleted the songx--include-fullName branch September 14, 2018 17:40
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants