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: reformat human readable output for test results #377

Closed
wants to merge 10 commits into from

Conversation

k-capehart
Copy link
Contributor

@k-capehart k-capehart commented Jun 10, 2024

What does this PR do?

Adds a verbose concise parameter to the HumanReporter format function. If true, then display all test results (current behavior). If false, only display failing tests. I have left the default value as true false to preserve existing behavior, with the plan to add a --verbose --concise flag to the sf apex run test command to control this behavior.

I have also moved the summary section for test results to the bottom of the output. This makes it much easier to tell at a glance whether your tests ran successfully or not. Even with the removal of passing tests in the output, it can still be cumbersome to scroll through the results to see the summary.

I primarily have the sf cli in mind but I don't know if this is used anywhere else or if there are other considerations I'm unaware of.

What issues does this PR fix or reference?

Functionality Before

Before, the summary was at the top of the terminal output and it displayed the results of ALL test methods, which required users to scroll a lot.

Functionality After

Now, the summary is at the bottom and can optionally only display failing tests, making it easier to see when read by a human.

@k-capehart k-capehart changed the title feat: move summary to bottom of output got human test reporter feat: move summary to bottom of output for human test reporter Jun 11, 2024
@k-capehart k-capehart marked this pull request as draft June 11, 2024 16:01
@k-capehart k-capehart changed the title feat: move summary to bottom of output for human test reporter feat: reformat human readable output for test results Jun 11, 2024
@k-capehart k-capehart marked this pull request as ready for review June 11, 2024 16:42
@peternhale
Copy link
Collaborator

@k-capehart We have taken a look at the PR and we really like the notion of displaying the summary last for the human reporter. We would like to move forward with this change with the following recommendations.

  • make the same change to humanFormatTransform.ts - we have recently added streaming reporters to better handle large test/coverage results.
  • change the new verbose parameter to concise
  • to protect backward compatibility concise should default false
  • human output with concise equal to true will produce in this order
    • test failures only
    • no code coverage
    • summary
  • human output with concise equal to false will produce in this order
    • all test results
    • code coverage
    • summary

Let us know if you have any questions and we do appreciate the contribution.

@k-capehart
Copy link
Contributor Author

@peternhale That's great news! I'll definitely make those changes when I get the time and let you know.

There were some concerns raised here about functionality within the CLI and making the same changes for JSON output. Is that still something I should be considering? #243 (comment)

@peternhale
Copy link
Collaborator

@peternhale That's great news! I'll definitely make those changes when I get the time and let you know.

There were some concerns raised here about functionality within the CLI and making the same changes for JSON output. Is that still something I should be considering? #243 (comment)

For this feature, let's restrict the changes to just the human format reporters. All other formatters, include JSON should remain as is.

@k-capehart
Copy link
Contributor Author

@peternhale Hello! I made the changes as requested. I didn't see any existing tests for HumanFormatTransform but let me know if I should add some.

@peternhale
Copy link
Collaborator

@peternhale Hello! I made the changes as requested. I didn't see any existing tests for HumanFormatTransform but let me know if I should add some.

Hmm, my bad. I would be nice to have tests ;), thanks in advance!

@k-capehart
Copy link
Contributor Author

@peternhale Will do 👍

Also, I think I missed something anyways. Your message mentions that if concise = true then code coverage shouldn't display. Is that the case even if detailedCoverage = true as well? Which flag takes priority?

@peternhale
Copy link
Collaborator

@k-capehart detail-coverage flag asks that test results have code coverage applied from the per class codecoverage. Results look like this

sf apex run test -c -w5 --detailed-coverage
=== Test Summary
NAME                 VALUE                        
───────────────────  ─────────────────────────────
Outcome              Passed                       
Tests Ran            11                           
Pass Rate            100%                         
Fail Rate            0%                           
Skip Rate            0%                           
Test Run Id          7075300007sfP8p              
Test Execution Time  2410 ms                      
Org Id               00D53000001GiuYEAS           
Username             [email protected]
Org Wide Coverage    93%                          


=== Apex Code Coverage for Test Run 7075300007sfP8p
TEST NAME                                                 CLASS BEING TESTED    OUTCOME  PERCENT  MESSAGE  RUNTIME (MS)
────────────────────────────────────────────────────────  ────────────────────  ───────  ───────  ───────  ────────────
TestPropertyController.testGetPagedPropertyList           PropertyController    Pass     75%               997         
TestPropertyController.testGetPagedPropertyList           PagedResult           Pass     100%              997         
TestPropertyController.testGetPicturesNoResults           PagedResult           Pass     100%              37          
TestPropertyController.testGetPicturesNoResults           PropertyController    Pass     20%               37          
TestPropertyController.testGetPicturesWithResults         PropertyController    Pass     30%               376         
TestPropertyController.testGetPicturesWithResults         PagedResult           Pass     100%              376         
GeocodingServiceTest.blankAddress                         GeocodingService      Pass     50%               15          
GeocodingServiceTest.errorResponse                        GeocodingService      Pass     75%               3           
GeocodingServiceTest.successResponse                      GeocodingService      Pass     86%               4           
TestSampleDataController.importSampleData                 SampleDataController  Pass     100%              294         
FileUtilitiesTest.createFileFailsWhenIncorrectBase64Data  FileUtilities         Pass     50%               111         
FileUtilitiesTest.createFileFailsWhenIncorrectFilename    FileUtilities         Pass     50%               29          
FileUtilitiesTest.createFileFailsWhenIncorrectRecordId    FileUtilities         Pass     78%               248         
FileUtilitiesTest.createFileSucceedsWhenCorrectInput      FileUtilities         Pass     83%               296         


=== Apex Code Coverage by Class
CLASSES               PERCENT  UNCOVERED LINES
────────────────────  ───────  ───────────────
SampleDataController  100%                    
PropertyController    100%                    
GeocodingService      100%                    
PagedResult           100%                    
FileUtilities         94%      35             

I then intentionally fail a test and run the tests again and I see

sf apex run test -c -w5 --detailed-coverage
 ›   Warning: Plugin @salesforce/plugin-apex (3.1.18) differs from the version specified by sf (3.1.14)
=== Test Summary
NAME                 VALUE                        
───────────────────  ─────────────────────────────
Outcome              Failed                       
Tests Ran            11                           
Pass Rate            91%                          
Fail Rate            9%                           
Skip Rate            0%                           
Test Run Id          7075300007sfPP3              
Test Execution Time  2313 ms                      
Org Id               00D53000001GiuYEAS           
Username             [email protected]
Org Wide Coverage    93%                          


=== Apex Code Coverage for Test Run 7075300007sfPP3
TEST NAME                                                 CLASS BEING TESTED    OUTCOME  PERCENT  MESSAGE                                                                            RUNTIME (MS)
────────────────────────────────────────────────────────  ────────────────────  ───────  ───────  ─────────────────────────────────────────────────────────────────────────────────  ────────────
TestPropertyController.testGetPagedPropertyList           PagedResult           Pass     100%                                                                                        957         
TestPropertyController.testGetPagedPropertyList           PropertyController    Pass     75%                                                                                         957         
TestPropertyController.testGetPicturesNoResults           PagedResult           Pass     100%                                                                                        38          
TestPropertyController.testGetPicturesNoResults           PropertyController    Pass     20%                                                                                         38          
TestPropertyController.testGetPicturesWithResults         PropertyController    Pass     30%                                                                                         364         
TestPropertyController.testGetPicturesWithResults         PagedResult           Pass     100%                                                                                        364         
GeocodingServiceTest.blankAddress                         GeocodingService      Pass     50%                                                                                         13          
GeocodingServiceTest.errorResponse                        GeocodingService      Pass     75%                                                                                         3           
GeocodingServiceTest.successResponse                      GeocodingService      Fail     86%      System.AssertException: Assertion Failed: Assertion failed: I broke it on purpose  
                                                                                                  External entry point                                                               
                                                                                                  Class.GeocodingServiceTest.successResponse: line 35, column 1                      4           
TestSampleDataController.importSampleData                 SampleDataController  Pass     100%                                                                                        276         
FileUtilitiesTest.createFileFailsWhenIncorrectBase64Data  FileUtilities         Pass     50%                                                                                         97          
FileUtilitiesTest.createFileFailsWhenIncorrectFilename    FileUtilities         Pass     50%                                                                                         25          
FileUtilitiesTest.createFileFailsWhenIncorrectRecordId    FileUtilities         Pass     78%                                                                                         256         
FileUtilitiesTest.createFileSucceedsWhenCorrectInput      FileUtilities         Pass     83%                                                                                         280         


=== Apex Code Coverage by Class
CLASSES               PERCENT  UNCOVERED LINES
────────────────────  ───────  ───────────────
SampleDataController  100%                    
PropertyController    100%                    
GeocodingService      100%                    
PagedResult           100%                    
FileUtilities         94%      35             

The detailed coverage for the failed test is still available, 86%.

So I would say these two flags are independent, concise only shows failed tests and suppresses "=== Apex Code Coverage by Class." The flag detailed-coverage augments each test with its associated coverage.

@k-capehart
Copy link
Contributor Author

@peternhale Thank you, that helps. So just to confirm, using the --concise flag with your last output should look like this:

sf apex run test -c -w5 --detailed-coverage --concise

=== Apex Code Coverage for Test Run 7075300007sfPP3
TEST NAME                                                 CLASS BEING TESTED    OUTCOME  PERCENT  MESSAGE                                                                            RUNTIME (MS)
────────────────────────────────────────────────────────  ────────────────────  ───────  ───────  ─────────────────────────────────────────────────────────────────────────────────  ────────────
GeocodingServiceTest.successResponse                      GeocodingService      Fail     86%      System.AssertException: Assertion Failed: Assertion failed: I broke it on purpose  
                                                                                                  External entry point                                                               
                                                                                                  Class.GeocodingServiceTest.successResponse: line 35, column 1                      4           

=== Test Summary
NAME                 VALUE                        
───────────────────  ─────────────────────────────
Outcome              Failed                       
Tests Ran            11                           
Pass Rate            91%                          
Fail Rate            9%                           
Skip Rate            0%                           
Test Run Id          7075300007sfPP3              
Test Execution Time  2313 ms                      
Org Id               00D53000001GiuYEAS           
Username             [email protected]
Org Wide Coverage    93%  

@k-capehart
Copy link
Contributor Author

@peternhale Ok, I think things should be good now. Looks like you already saw my comments but let me know if I missed anything.

@peternhale
Copy link
Collaborator

@k-capehart Thanks for the updates and we appreciate the contribution. I will try to get to a full review and my testing by COB tomorrow.

@k-capehart
Copy link
Contributor Author

Thanks for the help!

@peternhale
Copy link
Collaborator

@k-capehart I found some time to review and test these changes.

I did find one case that is producing undesired results in HumanReporter. It occurs when running with concise true and no test failures. The output contains undefined in the text results. I am attaching a zip that contains a runTest.ts file along with the text of two runs, with test failures and without test failures.
pr_testing.zip

@k-capehart
Copy link
Contributor Author

@peternhale Thanks for catching that. It should be fixed now. I ran your tests and no longer see 'undefined'.

@peternhale
Copy link
Collaborator

@k-capehart I approved the PR and I will merge soon. Thanks again!

plugin-apex should pick up the change automatically in the coming weeks

FYI @mshanemc

@k-capehart
Copy link
Contributor Author

@peternhale Awesome, thanks so much! If you need someone to work on the update for plugin-apex I'll be happy to submit a PR there as well once this update rolls out to it. I'll keep an eye on it.

@peternhale
Copy link
Collaborator

If you want it sooner than later then all it should require is picking up the version of apex-node.
I am trying to figure out why windows tests are failing for HumanFormatTransform test should format test results and not display coverage results if concise is true, so might be tomorrow sometime for the merge.

@peternhale
Copy link
Collaborator

@k-capehart I am going to close this PR. I had to fix a formatting error in human transform (that your test found, thanks again). That change and your changes from this PR have been merged in PR #383.

Thanks again for this contribution.

@peternhale peternhale closed this Jun 21, 2024
@k-capehart
Copy link
Contributor Author

@peternhale Awesome! I'm happy to contribute, thanks for you assistance and support in getting it out.

peternhale added a commit that referenced this pull request Jun 21, 2024
force a release for new feature
@W-16047075@
peternhale added a commit that referenced this pull request Jun 21, 2024
force a release for new feature
@W-16047075@
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants