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

Cypress initial cleanup pass #16189

Merged
merged 4 commits into from
Oct 14, 2022
Merged

Cypress initial cleanup pass #16189

merged 4 commits into from
Oct 14, 2022

Conversation

ambirdsall
Copy link
Contributor

@ambirdsall ambirdsall commented Aug 31, 2022

I found running the cypress test suite to be pretty confusing and not well-documented; and when I got that working, I noticed that base.spec.ts was always failing due to uncaught (but intentionally-triggered) errors in the application's js runtime. That can be straightforwardly fixed with a spec-local handler for the uncaught:exception event (there are ways to register a global event handler for uncaught exceptions, but generally we want errors to fail tests).

This is only a very quick initial pass at improving the developer experience for our Cypress tests. Here are a few follow-up tasks I'd like to tackle soon:

  • run the prettier command to reformat all test typescript files according to our conventions. I think this should wait until @SofiiaZaitseva has finished and merged 15700 add tests for PokeAPI #15701, to avoid creating a huge pile of merge conflicts for that PR; it should also be an isolated PR, because the file diffs from automated reformatting will be long and noisy, and would make any changes to the code's behavior unnecessarily hard to review.
  • create a wrapper script to starting up all the server processes required for a local test run without so many manual steps.

I'd also like to create some custom cypress commands to give ourselves a streamlined, easy-to-use API for airbyte-specific logic and DOM interactions, but that's a less discrete/well-scoped task.

@ambirdsall ambirdsall marked this pull request as ready for review August 31, 2022 20:19
@ambirdsall ambirdsall requested a review from a team as a code owner August 31, 2022 20:19
Copy link
Contributor

@krishnaglick krishnaglick left a comment

Choose a reason for hiding this comment

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

Is the expectation that, assuming no disabled UI, localhost:8000 is available and with a disabled UI & npm start, localhost:3000 is used?
Does docker-compose stop webapp need to be run after the OSS backend has been spun up?

@ambirdsall
Copy link
Contributor Author

Is the expectation that, assuming no disabled UI, localhost:8000 is available and with a disabled UI & npm start, localhost:3000 is used? Does docker-compose stop webapp need to be run after the OSS backend has been spun up?

Yes to both. As a result of this comment, I dug a little deeper, and will rewrite this a bit; some things that I phrased as options are actually required. The TL;DR is that you must use localhost:3000 (i.e. an npm started frontend) with npm run cypress:open and you must use localhost:8000 (i.e. the docker-compose started frontend) with npm run cypress:ci and npm run cypress:ci:record.

Background info: unless given a fully-qualified url, Cypress uses a baseUrl setting to build the urls it browses and sends HTTP requests to. The default baseUrl is set in cypress.json, but you can override that with an environment variable. So, for instance, if you wanted to run tests against the dockerized frontend, you'd do that with CYPRESS_BASE_URL=http://localhost:8000 npm run cypress:open. The two cypress:ci* npm scripts both do exactly that.

@ambirdsall
Copy link
Contributor Author

ambirdsall commented Sep 2, 2022

Cypress doesn't actually know or care anything about the backend--but if the UI can't reach any server, everything fails. You can actually run cypress against a cloud backend, given a reliable network connection, but the tests contain a bunch of implicit assumptions that they're interacting with an OSS build, so again, lots of failures.

As for docker-compose stop webapp, I was mainly following the lead of Airbyte's "Developing locally" documentation in recommending it. It's not actually necessary: there's no conflict if you run both frontend servers, but since you have to run npm start anyway, the dockerized one is just a waste of RAM.

Copy link
Contributor

@krishnaglick krishnaglick left a comment

Choose a reason for hiding this comment

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

Really nicely documented! 👍

@ambirdsall ambirdsall merged commit 1d2a4ba into master Oct 14, 2022
@ambirdsall ambirdsall deleted the alex/cypress-first-pass branch October 14, 2022 16:22
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
* Update README with instructions

* Prevent test failures from intentionally-thrown errors

* Rephrase cypress open instructions in README

* Add CI repro documentation
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