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

Increase test coverage #497

Merged
merged 3 commits into from
Mar 13, 2024
Merged

Increase test coverage #497

merged 3 commits into from
Mar 13, 2024

Conversation

matteosz
Copy link
Contributor

This PR increase the test coverage for the modules util/encoding and util/encryption

@matteosz matteosz self-assigned this Feb 15, 2024
@CLAassistant
Copy link

CLAassistant commented Feb 15, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

🔒 Could not start CI tests due to missing safe PR label. Please contact a DEDIS maintainer.

@matteosz matteosz marked this pull request as ready for review February 15, 2024 10:30
Copy link
Contributor

@AnomalRoil AnomalRoil left a comment

Choose a reason for hiding this comment

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

LGTM, just one comment.

util/encoding/encoding_test.go Outdated Show resolved Hide resolved
@matteosz matteosz marked this pull request as draft February 16, 2024 13:37
@matteosz matteosz marked this pull request as ready for review February 16, 2024 13:37
AnomalRoil
AnomalRoil previously approved these changes Feb 19, 2024
Copy link
Contributor

@pierluca pierluca left a comment

Choose a reason for hiding this comment

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

Good increment, I've suggested some improvements :)

util/random/rand_test.go Show resolved Hide resolved
return 0, nil
}

func TestReadHexPointError(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to be annoying on a point that's not common in this codebase, that is a bit of a pet peeve of mine, but which I think is important nonetheless:

This test contains 3 different test cases. When possible, try to have independent unit tests.

  • This way, when the test fails, it tells you directly that it's the "reader fails" that fails, you don't have to look up which line this failed at.
  • Furthermore, if the 3rd test case passes, you won't be able to tell (from the test results) if everything's grouped in a single test.
  • Finally, having separate tests makes them more (trivially) independent of one another, and potentially parallelizable (not that it matters here)

Copy link

sonarcloud bot commented Mar 11, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@AnomalRoil AnomalRoil merged commit e1d90cf into master Mar 13, 2024
8 checks passed
@AnomalRoil AnomalRoil deleted the matt-increase-test-coverage branch March 13, 2024 10:29
K1li4nL pushed a commit that referenced this pull request May 16, 2024
of util/encoding and util/encryption
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants