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.

Sign up for GitHub

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

Jump to bottom

Added test for grpc UnaryInterceptorClient #695

Merged
merged 7 commits into from
May 15, 2020
Merged

Added test for grpc UnaryInterceptorClient #695

merged 7 commits into from
May 15, 2020

Conversation

mujtaba-ahmed12
Copy link
Contributor

This address issue #684

@mujtaba-ahmed12 mujtaba-ahmed12 force-pushed the grpctest branch 3 times, most recently from 0bfd584 to 67e5188 Compare May 2, 2020 22:25
MrAlias
MrAlias requested changes May 4, 2020
View reviewed changes
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Good start.

Are there plans to include test for grpctrace.go as well?

plugin/grpctrace/interceptor_test.go Outdated Show resolved Hide resolved
plugin/grpctrace/interceptor_test.go Show resolved Hide resolved
plugin/grpctrace/interceptor_test.go Outdated Show resolved Hide resolved
plugin/grpctrace/interceptor_test.go Outdated Show resolved Hide resolved
plugin/grpctrace/interceptor_test.go Show resolved Hide resolved
plugin/grpctrace/interceptor_test.go Show resolved Hide resolved
plugin/grpctrace/interceptor_test.go Outdated Show resolved Hide resolved
@mujtaba-ahmed12
Copy link
Contributor Author

Good start.

Are there plans to include test for grpctrace.go as well?

Yes, that's what I am working now. I opened the pull for UrinaryInterceptor first so #683 could be unblocked and I add more test cases afterwards.

@MrAlias
Copy link
Contributor

MrAlias commented May 4, 2020

@mujtaba-ahmed12 would you be okay with prioritizing merging #683 prior to this?

@mujtaba-ahmed12
Copy link
Contributor Author

@mujtaba-ahmed12 would you be okay with prioritizing merging #683 prior to this?

What I meant was first merge this pull request, so #683 can rebase and merge its bug fix. I will add new test cases afterward. I will submit patch for changes requested today.

@MrAlias
Copy link
Contributor

MrAlias commented May 7, 2020

@mujtaba-ahmed12 #683 is not blocked on this work and contains a bug fix so I'm going prioritize merging that. If you need help resolving the ensuing merge conflicts let me know and I'll be happy to help.

@mujtaba-ahmed12 mujtaba-ahmed12 force-pushed the grpctest branch 4 times, most recently from 9fdb5a5 to 0b3ea3c Compare May 11, 2020 18:51
@mujtaba-ahmed12
Copy link
Contributor Author

@MrAlias Can you review this today? I have rebased branch on top of master and added test for streamInterceptor.

MrAlias
MrAlias reviewed May 11, 2020
View reviewed changes
plugin/grpctrace/interceptor_test.go Outdated Show resolved Hide resolved
plugin/grpctrace/interceptor_test.go Outdated Show resolved Hide resolved
plugin/grpctrace/interceptor_test.go Outdated Show resolved Hide resolved
plugin/grpctrace/interceptor_test.go Show resolved Hide resolved
plugin/grpctrace/interceptor_test.go Show resolved Hide resolved
plugin/grpctrace/interceptor_test.go Show resolved Hide resolved
plugin/grpctrace/interceptor_test.go Show resolved Hide resolved
@jmacd
Copy link
Contributor

jmacd commented May 13, 2020

@MrAlias or @mujtaba-ahmed12 please resolve the comments if this is ready to merge?

@MrAlias MrAlias changed the title Added test for grpc UrinaryInterceptorClient Added test for grpc UnaryInterceptorClient May 13, 2020
@MrAlias MrAlias linked an issue May 14, 2020 that may be closed by this pull request
Add tests to the grpctrace package #684
Closed
@mujtaba-ahmed12
Copy link
Contributor Author

@MrAlias Can you review this today, I have added condition for missing attributes in expected.

MrAlias
MrAlias approved these changes May 15, 2020
View reviewed changes
Copy link
Contributor

@MrAlias MrAlias 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. Thanks for all the hard work! 🎉

@MrAlias MrAlias merged commit 47d7132 into open-telemetry:master May 15, 2020
@MrAlias MrAlias mentioned this pull request May 15, 2020
Closed
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrAlias MrAlias MrAlias approved these changes

@jmacd jmacd jmacd approved these changes

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@krnowak krnowak Awaiting requested review from krnowak

@lizthegrey lizthegrey Awaiting requested review from lizthegrey

@paivagustavo paivagustavo Awaiting requested review from paivagustavo

@tedsuo tedsuo Awaiting requested review from tedsuo

@evantorrie evantorrie Awaiting requested review from evantorrie

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
untracked
Development

Successfully merging this pull request may close these issues.

Add tests to the grpctrace package
4 participants
@mujtaba-ahmed12 @MrAlias @jmacd @pellared