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

Include simple e2e test to test searching #978

Merged
merged 5 commits into from
Oct 1, 2021
Merged

Include simple e2e test to test searching #978

merged 5 commits into from
Oct 1, 2021

Conversation

zalegrala
Copy link
Contributor

What this PR does:

Extend the microservices e2e test to include search functionality.

Which issue(s) this PR fixes:
Fixes #925

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@zalegrala
Copy link
Contributor Author

Are the e2e tests run as part of CI?

@zalegrala
Copy link
Contributor Author

It appears so. make test-all seems to cover it.

joe-elliott
joe-elliott reviewed Sep 22, 2021
View reviewed changes
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

definitely remove the stringPointer() method. i will leave to @annanay25 to approve/merge, but it generally lgtm

annanay25
annanay25 reviewed Sep 23, 2021
View reviewed changes
Copy link
Contributor

@annanay25 annanay25 left a comment

Choose a reason for hiding this comment

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

the blind [0] deferences is kind of gorpy

I agree with joe this is kind of weird. how about we loop through all tags in every trace and search for them individually? might require that we make the tags/values unique

but honestly this whole test is

what else can we improve about this test?

integration/e2e/e2e_test.go Outdated Show resolved Hide resolved
@zalegrala
Copy link
Contributor Author

In looking over what we're doing, I think there are a few improvements we could make to the tests overall. Since we are making a bunch of http calls and inspecting the response code, we could also inspect the output and such. In the vulture tests, we're validating the content against a fixture, and we could do something similar here. If some of the vulture code were moved out of main_test.go we could reference and use it in these e2e tests that might smooth it out a little bit and gorpy-- somewhat. What do you think?

@zalegrala zalegrala force-pushed the e2eSearch branch 5 times, most recently from 90b7b90 to 23cdfd0 Compare September 29, 2021 15:01
annanay25
annanay25 approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@annanay25 annanay25 left a comment

Choose a reason for hiding this comment

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

Nice work @zelagrala, left one non-blocking comment.

integration/e2e/e2e_test.go Show resolved Hide resolved
pkg/util/trace_info.go Show resolved Hide resolved
@annanay25 annanay25 merged commit 6e94c1f into grafana:main Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe-elliott joe-elliott joe-elliott left review comments

@annanay25 annanay25 annanay25 approved these changes

@dgzlopes dgzlopes Awaiting requested review from dgzlopes

@kvrhdn kvrhdn Awaiting requested review from kvrhdn kvrhdn is a code owner

@mapno mapno Awaiting requested review from mapno mapno is a code owner

@mdisibio mdisibio Awaiting requested review from mdisibio mdisibio is a code owner

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

Successfully merging this pull request may close these issues.

Add e2e integration test for search
3 participants
@zalegrala @joe-elliott @annanay25