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

Get rid of verbose test runs #590

Merged
merged 2 commits into from
Feb 11, 2022

Conversation

mmlb
Copy link
Contributor

@mmlb mmlb commented Feb 11, 2022

Description

This is mostly about getting rid of the -v for all the go test runs.
Also gets rid of the cleaning the go test cache and adds some empty lines to ci.yaml.

Why is this needed

I got rid of -v as its just not necessary and I feel actually hinders debugging failed tests. When the tests pass I don't really care to see all the tests passing, pretty sure everyone just takes it for granted that all the tests are run :D. But when a test fails then the -v actually hinders debugging because now you have to go and hunt for the failed test and it's output. Without the -v go does the sane thing of only showing the verbose output of the failed test(s). So lets just get rid of the forced -v.

I got rid of the go clean -testcache calls because I have not found it to be necessary in my day-to-day development. Pretty sure a required use of go clean -testcache would be considered a bug in the golang/go. This change was introduced in 8ec0d84 without any reason so I'll just assume there wasn't one.

How Has This Been Tested?

Lots of make test and go test ./... for a long time without any breakage.

How are existing users impacted? What migration steps/scripts do we need?

Faster test runs (by avoiding re-testing unchanged code) and easier to find/debug test failures.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

mmlb added 2 commits February 11, 2022 09:58
Why do we clean the test cache? It should not be necessary (I haven't found it be
necessary).

Also get rid of -v as its just noisy and never useful. When there is not test failure
I don't really care to see *all* the tests being run. Yet when there *is* a test failure
we potentially have to scroll over all the passing tests just to get to the output of the
failed one. Why fight go here? Go will expand the test output for the failed tests anyway
so its just doesn't make any sense to run with -v.

Signed-off-by: Manuel Mendez <[email protected]>
For easier parsing/reading/scanning.

Signed-off-by: Manuel Mendez <[email protected]>
@codecov
Copy link

codecov bot commented Feb 11, 2022

Codecov Report

Merging #590 (2e6fe74) into main (8282ae7) will not change coverage.
The diff coverage is n/a.

❗ Current head 2e6fe74 differs from pull request most recent head 8c6f92d. Consider uploading reports for the commit 8c6f92d to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #590   +/-   ##
=======================================
  Coverage   43.41%   43.41%           
=======================================
  Files          51       51           
  Lines        3107     3107           
=======================================
  Hits         1349     1349           
  Misses       1673     1673           
  Partials       85       85           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1337c42...8c6f92d. Read the comment docs.

@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Feb 11, 2022
@mergify mergify bot merged commit f815d7a into tinkerbell:main Feb 11, 2022
@mmlb mmlb deleted the easier-to-see-test-failures branch February 11, 2022 15:30
@displague displague added this to the 0.7.0 milestone Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants