forcedotcom / salesforcedx-apex

Salesforce Apex Node Library
BSD 3-Clause "New" or "Revised" License
19 stars 25 forks source link

Human formatted Apex test report shouldn't display individual PASSING test methods #243

Closed daveespo closed 1 month ago

daveespo commented 3 years ago

Some of our projects have upwards of 2000 test methods. When we run force:apex:test:run -w 50 -r human in our CI/CI scripts and there is a single failed test method, we see 1999 lines of "Pass" and we have to painfully try to hunt down the single test method with the Fail status.

In a prior version of the CLI, it would only report test failures which meant that for a 100% successful test run, it would only print the summary block with the outcome of the test run -- there's no need to print 2000 lines of noise in the log file telling me that each test method was successful

Something changed at some point within the past few months (basically at the point where passing -r human was required in order to see the detail of the test failures)

Solution 1 - silence test methods that Pass by default

Pros: This is the way it should be and the way just about every other test reporter I've ever seen in my life has worked. Users that want all Passing test methods to be enumerated can use the --verbose existing command line argument to enable.

Cons: This will change existing behavior

Solution 2 - Add a command line argument (eg. --not-so-verbose) to silence Pass test methods from the output

Pros: Preserves existing behavior

Cons: Requires updating existing CI/CD scripts to pass this new flag. Also, documenting and naming the flag to make it somehow compatible with the --verbose documentation could be challenging.

Additional context Also, it should be noted that in the Summary table, the "Pass rate" is a whole number (presumably using half-up rounding logic) which results in incongruous outcome:

=== Test Summary
NAME                 VALUE
───────────────────  ──────────────────────────
Outcome              Failed
Tests Ran            870
Pass Rate            100%
Fail Rate            0%
Skip Rate            0%
Test Run Id          7071F0000279qGm
Test Execution Time  280082 ms
Org Id               00D1F000000aI3mUAE
Username             robot.1631036532038@pt.com

Which is it? A failure or 100% pass? What actually happened is one test method failed but that still resulted in the "99.9%" getting rounded up to 100%

Version that I'm running

@oclif/plugin-autocomplete 0.3.0 (core)
@oclif/plugin-commands 1.3.0 (core)
@oclif/plugin-help 3.2.2 (core)
@oclif/plugin-not-found 1.2.4 (core)
@oclif/plugin-plugins 1.10.1 (core)
@oclif/plugin-update 1.4.0-3 (core)
@oclif/plugin-warn-if-update-available 1.7.0 (core)
@oclif/plugin-which 1.0.3 (core)
@salesforce/sfdx-plugin-lwc-test 0.1.7 (core)
@salesforce/sfdx-trust 3.6.0 (core)
alias 1.1.10 (core)
apex 0.2.8 (core)
auth 1.7.1 (core)
config 1.2.24 (core)
custom-metadata 1.0.12 (core)
data 0.6.1 (core)
etcopydata 0.6.4-Beta (beta)
generator 1.1.7 (core)
limits 1.2.1 (core)
org 1.7.0 (core)
salesforce-alm 52.3.1 (core)
schema 1.0.8 (core)
sfdx-cli 7.116.2 (core)
shane-sfdx-plugins 4.43.0
├─ @mshanemc/plugin-streaming 1.1.7
└─ @mshanemc/sfdx-sosl 1.1.0
source 1.0.12 (core)
telemetry 1.2.3 (core)
templates 52.1.0 (core)
user 1.4.0 (core)
daveespo commented 5 months ago

@mshanemc -- @k-capehart was so kind as to do the thing with PR#377 -- what is the process for getting this change considered?

It could be viewed as a breaking change .. but it's been repeated a million times over that human readable output should not be relied on to remain consistent .. so I think this would qualify as a permitted change.

k-capehart commented 5 months ago

@mshanemc -- @k-capehart was so kind as to do the thing with PR#377 -- what is the process for getting this change considered?

It could be viewed as a breaking change .. but it's been repeated a million times over that human readable output should not be relied on to remain consistent .. so I think this would qualify as a permitted change.

This particular change in the node library is not breaking because I've defaulted the verbose value to true.

However, if we do add a --verbose flag to the cli command then that would technically change behavior. I agree with you that it shouldn't be relied on to be consistent. Regardless, it might be worth opening a new issue in the CLI repository specifically for that part of the discussion, since it'll be a new PR and specific to that team. Its just dependent on this being merged first. https://github.com/forcedotcom/cli/issues

VivekMChawla commented 5 months ago

@k-capehart and @daveespo - Improving Apex test results is something I'd like to get onto our near-term roadmap.

Some of the suggestions I've seen include adding a --concise flag to produce output similar to what this PR suggests.

While our stated policy is that we reserve the right to change human-readable output at any time, the importance of Apex test results in automation use cases makes me want to tread carefully.

All the suggestions to improve Apex test results that I've seen would result in significant changes to human-readable output, and would be best delivered with matching changes to JSON output. So, if we're going to take a "breaking changes" hit, I'd like to solve as much as we can in the process.

To that end, I plan to start a GitHub discussion to consolidate existing feature requests and solicit new ideas. The CLI team will turn that into a design proposal for final feedback. Then, we'll build the changes and put the new behavior behind an environment variable for a period of time while the old behavior is deprecated.

FYI: @mshanemc

k-capehart commented 5 months ago

@VivekMChawla Thanks for the reply. I appreciate the transparency as always. I'll keep an eye out for that discussion post and for ways to help so we can get this out sooner rather than later.

k-capehart commented 4 months ago

@daveespo Just updating you. This is just pending the following PR: https://github.com/salesforcecli/plugin-apex/pull/504

Once its approved then this should be good to go!

dschach commented 4 months ago

Linking to https://github.com/forcedotcom/cli/discussions/2872 for reference

daveespo commented 3 months ago

Yep, I've been watching .. fingers-crossed on quick approval and merge ;-)

k-capehart commented 3 months ago

https://github.com/salesforcecli/plugin-apex/pull/553 was merged to main, which contains the commits from my PR. Should be good to go in the next week or two for the next release! 🚀

avesolovksyy commented 3 months ago

@k-capehart How about test results that are displayed as a part of sf force source deploy --checkonly --testlevel ... command? Seems like there is similar problem with output there and having --concise would be beneficial.

I haven't used yet sf project deploy start that is suggested as a replacement of to be deprecated deploy command above - hope that --concise flag there serves the same purpose, i.e. to display only failures right at the bottom of the screen.

k-capehart commented 3 months ago

Hey @avesolovksyy , I completely agree. I would recommend opening an Issue in forcedotcom/cli to request this feature be added to project deploy start.

jshackell-sfdc commented 1 month ago

This issue has been fixed in release 2.56.7 (August 28, 2024).