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.

Sign up for GitHub

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

Jump to bottom

lc/colorful human report output #209

Closed
wants to merge 5 commits into from
Closed

lc/colorful human report output #209

wants to merge 5 commits into from

Conversation

lukecotter
Copy link

@lukecotter lukecotter commented Jun 29, 2021 •
edited
Loading

What does this PR do?

Test Results

  • Pass tests outcomes shown as a green pass badge.
  • Fail tests outcome shown as a red pass badge.
  • Stack trace shown below the test name, in the same column.
    Easier to follow + parse and takes up less width in the terminal output.
  • Coverage shown in green if 75 or over, red otherwise

Summary

  • Summary moved to bottom of output with new Failed tests section below that.
    Allows user to determine if errors exists without scrolling to top for a long time. Useful if terminal has lots of previous output.
  • passed, failed + skipped figures show in green, red + yellow.
  • passed, failed + skipped values show as a number with the rate in brackets.
  • Coverage shown in green if 75 or over, red otherwise

Failed Tests
-New section in the Human Report which shows only the failed test at the bottom of the terminal output in the same format as Test Results section (if any exist).

I have already implemented the above feature in the related pull request.
It also makes two other changes

  • handle spaces between tests
    e.g
    --classnames=" ATest , SecondTest " is valid and not just
    --classnames="ATest,SecondTest"
  • column after multi row cell now aligned at top e.g
col 1               |  col2
------------  |  ---------
Some multi   | more text
row text        |

instead of

col 1               |  col2
------------  |  ---------
Some multi   | 
row text        | more text

What issues does this PR fix or reference?

Closes #208

Functionality Before

Screenshot 2021-06-29 at 09 52 54

Functionality After

Screenshot 2021-06-29 at 09 46 16

@lukecotter
feat: column after multi row cell now aligned at top
89e3690
@lukecotter
fix: test run cmd to handle spaces between tests
b94cba6 
e.g the following is now valid and previous is would have failed
`--classnames=" ATest , SecondTest "` is valid and not just
`--classnames="ATest,SecondTest"`
@lukecotter
feat: format human reporter with colors
7e829f1 
Test Results
- Pass tests outcomes shown as a green pass badge.
- Fail tests outcome shown as a red pass badge.
- Stack trace shown below the test name, in the same column.
Easier to follow + parse and takes up less width in the terminal output.
- Coverage shown in green if 75 or over, red otherwise

Summary
- Summary moved to bottom of output with new Failed tests section below that.
Allows user to determine if errors exists without scrolling to top for a long time. Useful if terminal has lots of previous output.
- passed, failed + skipped figures show in green, red + yellow.
- passed, failed + skipped values show as a number with the rate in bracets.
- Coverage shown in green if 75 or over, red otherwise

- Failed Tests
-New section in the Human Report which shows only the failed test at the bottom of the termnal output in the same format as Test Results section (if any exist).
@lukecotter lukecotter requested a review from a team as a code owner June 29, 2021 08:46
@AnanyaJha
Copy link
Contributor

AnanyaJha commented Jul 13, 2021 •
edited
Loading

Hi @lukecotter thank you so much for contributing! These changes look awesome - @smaddox-sf is our PM so I'll defer to her on making any updates to the human-readable output. @smaddox-sf any thoughts about making these updates to the output for the sfdx force:apex:test:run command?

@smaddox-sf
Copy link

Hi @lukecotter - Thank you for the willingness to contribute! In a previous version of these commands, we had color coding. However, adding the colors doesn't meet our accessibility standards, so colorizing the output is not an option.

@lukecotter
Copy link
Author

lukecotter commented Jul 26, 2021 •
edited
Loading

@smaddox-sf

Do you think the others changes are useful? I can remove the colouring and leave the rest if it is only colours that are the problem.

Alternatively I can add a flag --no-color so colours can be disabled for those who need them to be.

Let me know what you think.

@smaddox-sf
Copy link

@smaddox-sf

Do you think the others changes are useful? I can remove the colouring and leave the rest if it is only colours that are the problem.

Alternatively I can add a flag --no-color so colours can be disabled for those who need them to be.

Let me know what you think.

Hi @lukecotter - Thanks for the willingness to work with us on this one!! Adding the Test Failures would be a great benefit along with some of the other improvements. We would need a couple other changes to move forward:

  1. We have to keep the "Messages" column and maintain the stack trace info in its own column vs. on a new line. Our standard for CLI commands is to have tabular output with headers and each piece of data in a distinct column. I agree this looks worse on smaller screens but we need to balance with overall CLI consistency.
  2. Let's move the "Test Summary" section to the end. You make a great point on having this readily available without scrolling. It would be better to have it at the end vs. between the "Test Results" and "Failed Tests" sections. Otherwise, if there are lots of test failures, you may still be scrolling to try to find the summary in the middle of the other tables.

The rest of the improvements look great (pending our internal testing and an engineering review of the code, of course).

On the colors, the default command output cannot have the color coding. However, we could add an optional flag to add the colors. I agree they look nice but we must lead with accessibility. If you are interested in this approach, we'll take it to our internal CLI review board to work out the flag name and can come back to you with the approved name.

@jag-j jag-j changed the base branch from develop to main December 6, 2021 23:41
@jag-j jag-j changed the base branch from main to develop December 6, 2021 23:41
@jag-j
Copy link
Contributor

jag-j commented Dec 6, 2021

@lukecotter - Since we are simplifying our branch strategy and deleting our 'develop' branch, we will have to close this PR for now. We would love to revisit to include these changes in 'main'.

@jag-j jag-j closed this Dec 6, 2021
@lukecotter
Copy link
Author

@lukecotter - Since we are simplifying our branch strategy and deleting our 'develop' branch, we will have to close this PR for now. We would love to revisit to include these changes in 'main'.

I forgot I needed to make changes, I will revisit in the new year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcoringrato-sfdc rcoringrato-sfdc Awaiting requested review from rcoringrato-sfdc

Assignees
No one assigned
Labels
cla:signed status:waiting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Colourful output for human resultformat
4 participants
@lukecotter @AnanyaJha @smaddox-sf @jag-j