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

Update frontend and integration tests #905

Conversation

danielbdias
Copy link
Contributor

@danielbdias danielbdias commented May 26, 2023

Changes

Today the integration and frontend tests are broken because they validate only 9 products sent by the ProductCatalog, causing our tests to fail when running make run-tests.

Also, the email test returns a false positive because we have a validation t.truthy(true) on this test. I updated the test data and changed this validation to t.truthy(res.ok) to guarantee that this endpoint is returning an HTTP 200 answer.

This PR fixes the assertions to consider the product added on #656 too.

To run these tests locally, first, you need to build both images with the following command:

docker compose build frontendTests integrationTests

And later just run:

make run-tests

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
  • Appropriate documentation updates in the docs
  • Appropriate Helm chart updates in the helm-charts

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

@danielbdias danielbdias requested a review from a team May 26, 2023 17:15
Comment on lines +287 to +290
// TODO: this validation is always returning 2 instead of 0, probably because of
// the cache leakage scenario, it makes sense to create two validations
// one for the leakage and another to the "normal" condition?
//t.is(arrayIntersection(res.productIds, req.productIds).length, 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Folks, I commented on this assertion because I'm not sure what we can do with this assertion. Do you have any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

The cache leak scenario should never be exercised when tests are running so I'd make it work for the happy path and not the sad one

@github-actions
Copy link

github-actions bot commented Jun 3, 2023

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 Jun 3, 2023
@danielbdias danielbdias force-pushed the fix/frontend-and-integration-tests branch from 323424e to 26da8e7 Compare June 5, 2023 12:54
@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 Jun 13, 2023
@austinlparker
Copy link
Member

Any update on comments here?

@danielbdias
Copy link
Contributor Author

hi @austinlparker ! I was on vacation last week, I'm planning to tackle the issue with the ProductCatalog test in the next few days. I should have this PR ready to review again on Monday.

@danielbdias
Copy link
Contributor Author

Hi folks! Giving a heads up here, I'm still trying to fix the test mentioned in this comment.
Looking at the trace this test generates, it does not trigger the cache case, although it continues to fail, showing 2 or 3 products in common with the output.
I'm doing more tests this week, and I believe that I should have a solution until the next SIG meeting on Monday.

@danielbdias
Copy link
Contributor Author

danielbdias commented Jun 26, 2023

Hi folks! I'm still checking what happened with the failing integration test, and I'll need more time to tackle it.

Thinking about that, I'll close this PR to investigate it and reopen it later as soon as I have a solution for that. (I'll ping on CNCF Slack to get the context better for the error).

As the frontend tests are solved, I opened another PR just for them: #950

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.

2 participants