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

Recommendation service e2e testing #198

Merged
merged 9 commits into from
Jul 26, 2022
Merged

Recommendation service e2e testing #198

merged 9 commits into from
Jul 26, 2022

Conversation

mic-max
Copy link
Contributor

@mic-max mic-max commented Jul 10, 2022

Towards #195

Merge after: #196
(If anyone knows a good git workflow for creating a PR based off another one that hasn't yet been merged yet please let me know so I can make these PRs cleaner since this one include all the previous branches commits - all I did was git checkout -b recc-service-test while on the mic-max:e2e-tests branch` anyways... shoot me a dm 😄)

Changes

  • Delete the client.py, essentially a manual version of this test
  • Open port to localhost so we can connect to the container from the test program

The 1 test sends 5 product ids. this makes sure that the recommendation service always sends back the same array 4 product ids (random ordering) - it caps at 5 recommendations and wont include anything you sent in the request. I was originally going to test it just by including 1 id in the request but then its possible the service sends back 5 products that dont include the one in my request just by chance. So this way is more robust, still not perfect though since the random generator isn't seeded and more reasons I won't get into.

@mic-max mic-max requested a review from a team July 10, 2022 02:01
Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

I've tried the test and they failed in 4 out 5 tests, was that intended?

✖ payment: valid credit card Rejected promise returned by test
✖ payment: invalid credit card 
✖ payment: amex credit card not allowed 
✖ payment: expired credit card 
✔ recommendation: list products

test/README.md Outdated Show resolved Hide resolved
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jul 20, 2022
@mic-max
Copy link
Contributor Author

mic-max commented Jul 25, 2022

I've tried the test and they failed in 4 out 5 tests, was that intended?

✖ payment: valid credit card Rejected promise returned by test
✖ payment: invalid credit card 
✖ payment: amex credit card not allowed 
✖ payment: expired credit card 
✔ recommendation: list products

The payment service had an issue with it that I fixed in this PR which ill reduce to only the fix

Update: heres the new PR fixing those tests #230

Copy link
Contributor

@cartersocha cartersocha left a comment

Choose a reason for hiding this comment

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

LGTM

@julianocosta89
Copy link
Member

Got the following now:
image

Are we expecting only recommendation: list products to be valid in this PR?
If yes, I'm good to approve.

@mic-max
Copy link
Contributor Author

mic-max commented Jul 26, 2022

Are we expecting only recommendation: list products to be valid in this PR? If yes, I'm good to approve.

Correct. The payment test failures were fixed in this PR

@cartersocha cartersocha dismissed julianocosta89’s stale review July 26, 2022 18:48

Cleared up in comment above

@cartersocha cartersocha merged commit 30b2197 into open-telemetry:main Jul 26, 2022
@mic-max mic-max deleted the recc-service-test branch July 27, 2022 17:56
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
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.

3 participants