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

Add per call options to ygnmi #57

Merged
merged 5 commits into from
Aug 19, 2022
Merged

Add per call options to ygnmi #57

merged 5 commits into from
Aug 19, 2022

Conversation

DanG100
Copy link
Contributor

@DanG100 DanG100 commented Aug 18, 2022

No description provided.

@coveralls
Copy link

coveralls commented Aug 18, 2022

Pull Request Test Coverage Report for Build 2892189026

  • 85 of 89 (95.51%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 88.51%

Changes Missing Coverage Covered Lines Changed/Added Lines %
ygnmi/gnmi.go 54 58 93.1%
Totals Coverage Status
Change from base Build 2884702816: 0.2%
Covered Lines: 1533
Relevant Lines: 1732

💛 - Coveralls

Copy link
Contributor

@wenovus wenovus left a comment

Choose a reason for hiding this comment

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

Looks good, some minor comments

internal/testutil/gnmi.go Outdated Show resolved Hide resolved
internal/testutil/gnmi.go Outdated Show resolved Hide resolved
ygnmi/ygnmi.go Outdated Show resolved Hide resolved
ygnmi/gnmi.go Show resolved Hide resolved
ygnmi/ygnmi.go Outdated Show resolved Hide resolved
ygnmi/gnmi.go Show resolved Hide resolved
ygnmi/ygnmi.go Show resolved Hide resolved
ygnmi/ygnmi_test.go Outdated Show resolved Hide resolved
ygnmi/ygnmi_test.go Outdated Show resolved Hide resolved
internal/testutil/gnmi.go Outdated Show resolved Hide resolved
Copy link
Contributor

@wenovus wenovus left a comment

Choose a reason for hiding this comment

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

Thanks for the thorough tests

@DanG100 DanG100 merged commit 0b877c9 into main Aug 19, 2022
@DanG100 DanG100 deleted the opts branch August 19, 2022 21:19
stub *Stubber
agent *gnmi.Agent
stub *Stubber
getWrapper *getWrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this was the Ondatra naming, but maybe "clientWrapper" would be a better name, since it wraps the client? Or perhaps the field could be named "clientWrapper" and the struct named "clientWithGetter"

getRequests []*gpb.GetRequest
}

// Get is fake implement of gnmi.Get, it returns the GetResponse contained in the stub.
Copy link
Contributor

Choose a reason for hiding this comment

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

"is fake implement" -> "is a fake implementation"?

@@ -87,6 +116,12 @@ func (s *Stubber) Notification(n *gpb.Notification) *Stubber {
return s
}

// AppendGetResponse appends the given GetResponse as a stub response.
func (s *Stubber) AppendGetResponse(gr *gpb.GetResponse) *Stubber {
Copy link
Contributor

Choose a reason for hiding this comment

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

All the other methods on Stubber are named by what they stub/append and omit the word "append". Justification for this one breaking that pattern?

return nil
}

// Recv returns the result of the Get request, returning io.EOF after resonpse are read.
Copy link
Contributor

Choose a reason for hiding this comment

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

*response

@DanG100
Copy link
Contributor Author

DanG100 commented Aug 22, 2022

fixes in #58

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.

4 participants