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

Mock NTP client #217

Merged
merged 10 commits into from
Jan 18, 2023
Merged

Mock NTP client #217

merged 10 commits into from
Jan 18, 2023

Conversation

malancas
Copy link
Contributor

@malancas malancas commented Jan 13, 2023

Summary

Closes #148

Adds a mock NTP client so we can add more complete unit testing for the ntpmonitor package. The bulk of the logic in NTPMonitor#Start has been moved into NTPMonitor#queryServers so we can test the logic more easily.

Release Note

Documentation

Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2023

Codecov Report

Merging #217 (59f1ee0) into main (c8b76e6) will increase coverage by 4.61%.
The diff coverage is 86.11%.

@@            Coverage Diff             @@
##             main     #217      +/-   ##
==========================================
+ Coverage   48.05%   52.66%   +4.61%     
==========================================
  Files          19       19              
  Lines        1130     1143      +13     
==========================================
+ Hits          543      602      +59     
+ Misses        529      483      -46     
  Partials       58       58              
Impacted Files Coverage Δ
pkg/ntpmonitor/randomchoice.go 100.00% <ø> (ø)
pkg/ntpmonitor/ntpmonitor.go 64.60% <86.11%> (+50.60%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

hectorj2f
hectorj2f previously approved these changes Jan 17, 2023
Copy link
Contributor

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

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

lgtm

Signed-off-by: Meredith Lancaster <malancas@github.com>
@malancas malancas marked this pull request as ready for review January 17, 2023 15:12
@malancas malancas requested a review from a team as a code owner January 17, 2023 15:12
ntpClient NTPClient
}

func (n *NTPMonitor) WithCustomClient(client NTPClient) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd propose making a NewFromConfigWithClient method instead of WithCustomClient. Otherwise, you're initializing a client twice, once in NewFromConfig and once in WithCustomClient.

)

type MockNTPClient struct {
ignoredServers map[string]string
Copy link
Contributor

@haydentherapper haydentherapper Jan 17, 2023

Choose a reason for hiding this comment

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

What is ignoredServers? Maybe a comment?

Edit: Ah, used for which servers to fail on. Yea, a comment would be helpful!

@@ -160,7 +190,7 @@ func (n *NTPMonitor) QueryNTPServer(srv string) (*ntp.Response, error) {
opts := ntp.QueryOptions{
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Should QueryNTPServer be private, because we don't call it elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, looks like it isn't called anywhere else.

}
if !tc.expectTestToPass && err == nil {
t.Errorf("test '%s' unexpectedly passed with a nil error", tc.name)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also test metrics output? (I'm not sure!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into this.

Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

Nice, this looks great!

@malancas malancas merged commit d1012d2 into sigstore:main Jan 18, 2023
@malancas malancas deleted the mock-ntp-client branch January 18, 2023 15:11
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.

Implement a mock NTP client for testing.
4 participants