feat: reformat human readable output for test results

k-capehart commented 3 months ago

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.

peternhale commented 3 months ago

@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 commented 3 months ago

@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? https://github.com/forcedotcom/salesforcedx-apex/issues/243#issuecomment-2165342033

peternhale commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

@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             test-deyp9zualoy8@example.com
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             test-deyp9zualoy8@example.com
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 commented 3 months ago

@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             test-deyp9zualoy8@example.com
Org Wide Coverage    93%

k-capehart commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

Thanks for the help!

peternhale commented 3 months ago

@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 commented 3 months ago

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

peternhale commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

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 commented 2 months ago

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

k-capehart commented 2 months ago

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

forcedotcom / salesforcedx-apex