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

Add unit tests for testing hubble args/flags handling #874

Merged
merged 1 commit into from
Jan 26, 2023
Merged

Add unit tests for testing hubble args/flags handling #874

merged 1 commit into from
Jan 26, 2023

Conversation

chancez
Copy link
Contributor

@chancez chancez commented Jan 25, 2023

Add unit tests that just test the flag/command handling.

This should help prevent what happened in #851 (comment).

@chancez chancez added kind/enhancement This would improve or streamline existing functionality. 🤖 area/CI Impacts the testing / continuous integration testing of the project. labels Jan 25, 2023
@chancez chancez requested a review from a team as a code owner January 25, 2023 21:11
@chancez chancez requested review from kaworu and removed request for a team January 25, 2023 21:11
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label PR is blocked until the release note is set label Jan 25, 2023
@chancez chancez force-pushed the pr/chancez/hubble_cli_unit_tests branch 2 times, most recently from 950e9c5 to 3a714c5 Compare January 25, 2023 22:16
@chancez chancez requested review from gandro and rolinh January 25, 2023 22:46
@chancez chancez added the release-note/ci This PR makes changes to the CI. label Jan 26, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label PR is blocked until the release note is set label Jan 26, 2023
@chancez chancez mentioned this pull request Jan 26, 2023
Merged
gandro
gandro approved these changes Jan 26, 2023
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks, looks good overall! Glad to see coverage increased.

cmd/cli_test.go Show resolved Hide resolved
rolinh
rolinh requested changes Jan 26, 2023
View reviewed changes
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Awesome, I definitely welcome more tests!
Lgtm except for the bug that leads to a panic (see comment below).

cmd/observe/flows.go Outdated Show resolved Hide resolved
@chancez chancez force-pushed the pr/chancez/hubble_cli_unit_tests branch 2 times, most recently from d405618 to c4f8ecb Compare January 26, 2023 17:12
@chancez
Copy link
Contributor Author

chancez commented Jan 26, 2023

Hmm I just realized incidentally that this init setting GetHubbleClientFunc will potentially effect all the tests (even outside the package?), so I might revisit that approach first.

@chancez
Copy link
Contributor Author

chancez commented Jan 26, 2023

Actually, I may tackle that after #875 since that refactor will make removing the global a bit easier.

@chancez
Add unit tests for testing hubble args/flags handling
cc9b44b 
Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez chancez force-pushed the pr/chancez/hubble_cli_unit_tests branch from c4f8ecb to cc9b44b Compare January 26, 2023 17:23
@chancez chancez requested a review from rolinh January 26, 2023 17:58
@chancez chancez merged commit a07eaad into master Jan 26, 2023
@chancez chancez deleted the pr/chancez/hubble_cli_unit_tests branch January 26, 2023 23:17
@chancez chancez mentioned this pull request Jan 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@rolinh rolinh rolinh approved these changes

@kaworu kaworu Awaiting requested review from kaworu kaworu was automatically assigned from cilium/sig-hubble

Assignees
No one assigned
Labels
🤖 area/CI Impacts the testing / continuous integration testing of the project. kind/enhancement This would improve or streamline existing functionality. release-note/ci This PR makes changes to the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@chancez @gandro @rolinh