Skip to content
This repository has been archived by the owner on May 25, 2022. It is now read-only.

Add test that validates fingerprint.StartsWith when read from files #145

Merged

Conversation

djaglowski
Copy link
Member

No description provided.

@djaglowski djaglowski marked this pull request as ready for review May 18, 2021 19:09
@djaglowski djaglowski requested a review from a team May 18, 2021 19:09
@@ -212,4 +214,49 @@ func TestFingerprintStartsWith(t *testing.T) {
}
}

func TestFingerprintStartsWith_FromFile(t *testing.T) {
rand.Seed(112358)
Copy link
Member

Choose a reason for hiding this comment

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

This global may not work well if tests are run in parallel (do we?).
r := rand.New(rand.NewSource(112358)) and using r.Read() should be safer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Fixed.

@@ -212,4 +214,49 @@ func TestFingerprintStartsWith(t *testing.T) {
}
}

func TestFingerprintStartsWith_FromFile(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

I am not familiar with what the fingerprints and how they work so I don't understand what this test does. Perhaps that's fine and I am not supposed to understand. If you want me to understand though please add some comments to explain what it does :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's certainly not meant to be a secret. :)

I expanded the comments on the Fingerprint struct and its StartsWith method, and also added a high level explanation on the new test.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Thank you for the comments, very helpful.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants