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

LF-4026 Get Cypress tests passing on current state of integration #3100

Conversation

kathyavini
Copy link
Collaborator

@kathyavini kathyavini commented Jan 30, 2024

TODO:

  • Opening to test running in CI -- passed!!! 🎉
  • Refactor the beforeEach of each spec into a reusable function

Description

This is an update of PR #2931, addressing comments in that PR and getting the tests passing on the current state of integration by updating selectors and points of failure/flakiness. It has been running smoothly locally, no flakiness, for several days now so hopefully the same on GitHub!! 🤞

Notes on running locally within packages/end-to-end with npm run cypress:

  • Because of the spotlight interactions, the tests need to be run on a clean database, the way that they will be run in CI (I have been using a script that drops and re-migrates my db for the purposes of my own testing)
  • Similarly, there is a temporal dependency such that the crops test needs to be run before the tasks test. This will always be the case in CI (they are run sequentially) but I'll make a note in the code because eventually it would be much better if the specs could be run in total isolation
  • The root cause for both of those issues is that I couldn't successfully get conditional "interact-if-present" logic working. This would be first on my list for the next iteration of the tests, but for now I let it be because it only is relevant in local use, not CI
  • The specs are setup to run through multiple language users (there a variable that controls this and it's well-integrated into the tests) but then the GitHub workflow was hardcoded for the English user. Discussed this with @antsgar and English-only is preferred for now (it will also keep the tests short)

Jira link: https://lite-farm.atlassian.net/browse/LF-4026

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Passes test case
  • UI components visually reviewed on desktop view
  • UI components visually reviewed on mobile view
  • Other (please explain)

Run locally from within packages/end-to/end with refreshed database using npm run cypress

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The precommit and linting ran successfully
  • I have added or updated language tags for text that's part of the UI
  • I have added "MISSING" for all new language tags to languages I don't speak
  • I have added the GNU General Public License to all new files

No longer searchable like this since PR #2951
Also change assertion on Proceed button click which was flaky locally
Also gitignore coverage report directory
Test was suddenly failing at this step
… order

All of CheckTaskNavigation is included within CreateCleanTask
… of date wait call

Not sure what @markerJsRequest used to wait, but tests now fail at that point
Also add three skipped selectors
@kathyavini kathyavini self-assigned this Jan 30, 2024
@kathyavini kathyavini requested review from a team as code owners January 30, 2024 18:32
@kathyavini kathyavini requested review from antsgar and Duncan-Brain and removed request for a team January 30, 2024 18:32
cy.get(Selectors.ADD_TASK_LOCATION_CONTINUE).should('exist').and('not.be.disabled').click();

// This screen only present when this test file is run after crops.js
cy.get(Selectors.ADD_TASK_CROPS_CONTINUE).should('exist').and('not.be.disabled').click();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the cause of the temporal dependency between crops.js and tasks.js. There is an additional page in this multi-page form when a crop plan exists on the farm.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the future to make this more robust and ensure the tests are independent, we'll likely need to bake in the creation of a crop in the tasks test itself as a part of the test setup before assertions. We'll probably end up abstracting the code that does the crop creation on its own reusable command

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah that's a good idea! I was thinking the spec needed to be more flexible here, but instead the environment could have been made more predictable.


cy.get('[data-cy=addTask-cropsContinue]').should('exist').and('not.be.disabled').click();
// This screen only present when this test file is run after crops.js
cy.get(Selectors.ADD_TASK_CROPS_CONTINUE).should('exist').and('not.be.disabled').click();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As above

@Duncan-Brain
Copy link
Collaborator

I think you are still working on this... but I took a look early -- I have seen it has passed CI ! Nice!

My main question will be about the movement/deletion of the google maps intercept requests any extra info you can provide would be cool.

@@ -0,0 +1,45 @@
/*
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What got timeboxed here was writing this function in ts and accepting an array instead of a single additional file -- after wrangling with the cy.fixture() promises for a while I left it be. It would be good for those improvements to be made someday 🙂

@kathyavini
Copy link
Collaborator Author

kathyavini commented Jan 30, 2024

@Duncan-Brain hmmm I don't know if I have too much extra info there! Mostly it's the exact same as how Mika set it up and I've come to love the pattern. I think the only things I changed were:

  • There was a sudden flakiness (or maybe more like sudden failure) on the Farm Selection page so I tried intercepting the network request I was seeing in dev tools and I haven't seen a failed test there since 👍
  • There was one hardcoded cy.wait(500) on a farm map interaction, so I intercepted and waited instead (not a move but a duplication of what was used in another farm map test) This proved flakey (passed one run, failed the second), so I restored Mika's original code -- now I'm assuming that he had his reasons for the explicit wait!
  • There was one intercept of @markerJsRequest that I truly don't know the purpose of; by the time I was running the tests this was causing a failure (the cy.wait never concluded) so I removed it

This was a point of failure in 2nd CI run
Copy link
Collaborator

@antsgar antsgar left a comment

Choose a reason for hiding this comment

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

This is really great work! 🙌 we might need to figure out a way to get rid of the explicit waiting at some point if it ends up in flakiness, but for now we move on 🙂 (relatedly, I found this article that I really enjoyed about cy.wait https://filiphric.com/waiting-in-cypress-and-how-to-avoid-it)

@@ -0,0 +1 @@
.nyc_output
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this related to code coverage? We're not running any coverage reports right now, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes exactly! And running the tests locally regenerates this folder, with the contents a single JSON file containing this 😆

{}

Copy link
Collaborator

@Duncan-Brain Duncan-Brain left a comment

Choose a reason for hiding this comment

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

Thanks @kathyavini for the explaining those little changes!

Might be worth asking Mika about the marker thing or others for the future..... I agree I cant find any use of it in our frontend.

@Duncan-Brain Duncan-Brain added this pull request to the merge queue Jan 31, 2024
Merged via the queue into integration with commit 1892333 Jan 31, 2024
7 checks passed
@Duncan-Brain Duncan-Brain deleted the LF-4026-get-cypress-tests-passing-on-current-state-of-integration branch January 31, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants