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

Change unit tests to check for one thing #381

Merged
merged 1 commit into from Sep 24, 2019

Conversation

MVrachev
Copy link
Contributor

The unit tests should check for a single thing at a time.
This was not true for some the tests.

Signed-off-by: Martin Vrachev mvrachev@vmware.com

@codecov-io
Copy link

codecov-io commented Sep 18, 2019

Codecov Report

Merging #381 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #381   +/-   ##
=======================================
  Coverage   67.01%   67.01%           
=======================================
  Files           9        9           
  Lines         573      573           
=======================================
  Hits          384      384           
  Misses        170      170           
  Partials       19       19

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 915e9ee...9cfa01a. Read the comment docs.

Copy link
Member

@ccojocar ccojocar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite get what's the goal of this change. In my opinion, it is fine to have multiple warnings per check. I don't see where do you cover the tests which you removed.

@MVrachev
Copy link
Contributor Author

I don't quite get what's the goal of this change. In my opinion, it is fine to have multiple warnings per check. I don't see where do you cover the tests which you removed.

There are a few things.
I will demonstrate my points with the unit test for G301:

  • in the test os.Mkdir("/tmp/mydir", 0777) is used which has an error and os.Mkdir("/tmp/mydir", 0600) which hasn't. Those should be two seprated tests.
  • when the os.Mkdir and os.MkdirAll are used they are not checked for returned errors and this leads to other G104 errors
  • os.Mkdir and os.MkdirAll are two different instances of the same rule. They should be checked separately.

So, in summary, I think that one who creates a unit test for Gosec should:

  • make a unit test as simple as possible without simulating the same error twice in a unit test but instead split the test in two if he wants to check two different instances of a rule
  • use a unit tests to do only one of the two things: check for an error or check that Gosec doesn't report any errors
  • make sure that Gosec will catch errors only related to the rule which is tested

The unit tests should check for a single thing at a time.
This was not true for some the tests.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev
Copy link
Contributor Author

I don't see where do you cover the tests which you removed.

For G301: all three statements are checked in three different unit tests.

For G302: all four statements are checked in three different unit tests.

For G303: I just removed the file opening because it was redundant.

@ccojocar ccojocar merged commit b504783 into securego:master Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants