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

🌱 use ValidateTestReturn for SAST tests #3872

Merged
merged 3 commits into from
Feb 13, 2024

Conversation

spencerschrock
Copy link
Member

What kind of change does this PR introduce?

test cleanup

What is the current behavior?

we only checked the result score, which produces test failures without clear actionable feedback.
I discovered this during another refactor and it was hard to figure out what the issue was.

--- FAIL: Test_SAST (0.00s)
    --- FAIL: Test_SAST/SAST_checker_should_return_failed_status_when_no_PRs_are_found (0.00s)
        ~/checks/sast_test.go:319: Expected score 0, got -1 for SAST checker should return failed status when no PRs are found
    --- FAIL: Test_SAST/Successful_SAST_checker_should_return_success_status_for_sonarcloud (0.00s)
        ~/checks/sast_test.go:319: Expected score 10, got -1 for Successful SAST checker should return success status for sonarcloud

What is the new behavior (if this is a feature change)?**

We use our test helper: ValidateTestReturn, which provides much more actionable feedback when tests fail:

--- FAIL: Test_SAST (0.00s)
    --- FAIL: Test_SAST/SAST_checker_should_return_failed_status_when_no_PRs_are_found (0.00s)
        ~/checks/sast_test.go:342: SAST checker should return failed status when no PRs are found: (-expected +actual)  utests.TestReturn{
            - 	Error:         nil,
            + 	Error:         e"internal error: probe run failure: [create finding: finding.New: invalid: ID: read '', expected 'sastToolConfigured']",
            - 	Score:         0,
            + 	Score:         -1,
            - 	NumberOfWarn:  1,
            + 	NumberOfWarn:  0,
              	NumberOfInfo:  0,
              	NumberOfDebug: 0,
              }
  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

NONE

Special notes for your reviewer

The warn/info/debug counts were just taken as-is from the current state of main branch.

Does this PR introduce a user-facing change?

For user-facing changes, please add a concise, human-readable release note to
the release-note

(In particular, describe what changes users might need to make in their
application as a result of this pull request.)

NONE

previously, we only checked the result score.
test failures with this method dont produce as actionable feedback.

Signed-off-by: Spencer Schrock <[email protected]>
Copy link

codecov bot commented Feb 12, 2024

Codecov Report

Merging #3872 (56ae55a) into main (9b65bde) will decrease coverage by 6.70%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3872      +/-   ##
==========================================
- Coverage   75.47%   68.77%   -6.70%     
==========================================
  Files         235      235              
  Lines       15793    15793              
==========================================
- Hits        11919    10861    -1058     
- Misses       3133     4261    +1128     
+ Partials      741      671      -70     

@spencerschrock spencerschrock enabled auto-merge (squash) February 13, 2024 17:27
@spencerschrock spencerschrock merged commit 68d8d93 into ossf:main Feb 13, 2024
38 checks passed
@spencerschrock spencerschrock deleted the sast-compare-logger-messages branch February 13, 2024 17:36
fhoeborn pushed a commit to fhoeborn/scorecard that referenced this pull request Apr 1, 2024
* check logger counts for SAST tests

previously, we only checked the result score.
test failures with this method dont produce as actionable feedback.

Signed-off-by: Spencer Schrock <[email protected]>

* clarify test names and score constants used

Signed-off-by: Spencer Schrock <[email protected]>

---------

Signed-off-by: Spencer Schrock <[email protected]>
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