-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🐛 Fixing tests configuration #306
Conversation
@mviitane Do you mind taking a look? |
@xoscar I can still see the same problem when running with your changes. Just to be clear, I haven't installed cypress or ava on this machine beforehand, and I think I shouldn't have to, but is that correct?
|
@mviitane qq, did you run the build command for both the frontend and the integration tests from the latest commit? I'm not getting those errors 🤔 |
I ran |
@xoscar After re-building with the latest, I got the integrationTests working! However, the frontendTests still have the same problem.
|
Hey @mviitane thanks for the feedback, I figured out what the issue was, do you mind rebuilding the |
@xoscar Great, I got the tests now running! The frontend tests seem to have 2 tests failing, but I'm not sure if the problem is in the tests or the frontend. In the original issue, I also proposed to add some simple documentation to test/README.md about how to start the tests. What do you think about that? |
@mviitane do you mind taking another look, please rebuild the image and rerun it from the latest commit. I ran it multiple times and got all green |
@xoscar I got now all the frontend tests passing few times, but still at least half of the times some tests were failing. So some flakiness at least in my env. Integration tests are passing all the time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several things...
1 - Still getting some flakiness in frontend tests.
frontend-tests |
frontend-tests | Running: Checkout.cy.ts (1 of 3)
frontend-tests |
frontend-tests |
frontend-tests | Checkout Flow
frontend-tests | 1) should create an order with two items
frontend-tests |
frontend-tests |
frontend-tests | 0 passing (7s)
frontend-tests | 1 failing
frontend-tests |
frontend-tests | 1) Checkout Flow
frontend-tests | should create an order with two items:
frontend-tests | AssertionError: Timed out retrying after 4000ms: Expected to find element: `[data-cy="cart-item-count"]`, but never found it.
frontend-tests | at Context.eval (http://frontend:8080/__cypress/tests?p=cypress/e2e/Checkout.cy.ts:111:158)
frontend-tests |
frontend-tests |
frontend-tests |
frontend-tests |
frontend-tests | (Results)
frontend-tests |
frontend-tests | ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
frontend-tests | │ Tests: 1 │
frontend-tests | │ Passing: 0 │
frontend-tests | │ Failing: 1 │
frontend-tests | │ Pending: 0 │
frontend-tests | │ Skipped: 0 │
frontend-tests | │ Screenshots: 1 │
frontend-tests | │ Video: true │
frontend-tests | │ Duration: 6 seconds │
frontend-tests | │ Spec Ran: Checkout.cy.ts │
frontend-tests | └────────────────────────────────────────────────────────────────────────────────────────────────┘
frontend-tests |
frontend-tests |
frontend-tests | (Screenshots)
frontend-tests |
frontend-tests | - /app/cypress/screenshots/Checkout.cy.ts/Checkout Flow -- should create an order (1280x720)
frontend-tests | with two items (failed).png
frontend-tests |
frontend-tests |
frontend-tests | (Video)
frontend-tests |
frontend-tests | - Started processing: Compressing to 32 CRF
frontend-tests | - Finished processing: /app/cypress/videos/Checkout.cy.ts.mp4 (1 second)
Didn't see any errors in integration tests, however.
2 - Not clear how these tests should be run, there's no associated make
target for them or compose target that I could find? I ran them by doing a clean build + docker compose up
, then running docker compose up frontendTests
and docker compose up integrationTests
.
Hey @austinlparker & @mviitane sorry that it took me a while to come back to this PR. With the latest commit, the frontend E2E tests issues should be fixed and I also added an entry to the main Readme file around the test execution and such. Please take a look at let me know what you think. Thanks! |
Hello @xoscar, I'm using Ubuntu 22.04 and getting 2 failing tests. Both because there is no item in the cart. Here are the screenshots generated by cypress: When running the
Maybe those are responsible for the failure. |
frontend tests passing, integration tests failing, ubuntu 22.04
make run-tests giving an error as well -
|
Ok everyone, thank you for your patience. I think I finally fixed the tests. The warning at the beginning shouldn't be a problem, tests should pass either way. For the integration tests I fixed the ones related to the catalog changes. But there is one outstanding issue regarding the C++ service not returning the right amount of supported currencies. |
@cartersocha sorry for the late response, I took one week off 😅 I just updated the PR skipping the flaky tests. Let me know what you think. @austinlparker |
The frontend tests seem stable now
The integration tests run successfully when first starting the app with And then using another terminal |
@julianocosta89, @austinlparker could we get another pass of reviews here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xoscar with the changes on the shipping service, which now gets the quote from the quoteservice, in order to get the tests to succeed we need to do what @mviitane mentioned.
1st docker compose up
And then using another terminal
docker compose run integrationTests
We need to either update the README or update the the test to start the quoteservice
together with the test.
@julianocosta89 @austinlparker @cartersocha done 😄 I added the quote service as a dependency of the |
@cartersocha I think we are good to go with this one |
Needs to be rebased! @xoscar |
Curious - are folks trying to achieve "zero open PRs" today? 😆 |
@reyang done :) |
Wow!! I guess we might see for the 1st time that an OpenTelemetry repository reaching zero opening PR 🍾🍾🍾🥇🥇🥇 |
@xoscar one more time please? Im waiting on the merge button 😉 |
@reyang I can close my language update PR if needed lolol |
For the day |
@cartersocha I think it should be visible now |
* fixing tests configuration * adding custom docker file for FE tests * improving tests * multiple fixes and readme entry * multiple fixes and readme entry * fixing E2E tests * skipping flaky tests * adding the quote service as a dependency for the integration tests
Fixes #305.
Changes
There is an issue with some of the test configurations that weren't caught before merging the prev PR.