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

Run E2E tests only for frontend directory #933

Merged
merged 1 commit into from
Mar 14, 2023

Conversation

codemonkey800
Copy link
Collaborator

Description

#806 introduced a change where the frontend tests are run on every push. However, it makes more sense to run the E2E tests only when frontend changes are made, so this PR adds an event filter that checks if it's a push to main or to a pull request in the frontend/* directory

@codemonkey800 codemonkey800 added the cicd Release Label: Used for categorizing CICD changes in automated CI release notes label Mar 8, 2023
@richaagarwal
Copy link
Collaborator

@codemonkey800 Are there any scenarios where non-frontend changes might impact functionality tested as part of the e2e tests?

@codemonkey800
Copy link
Collaborator Author

@codemonkey800 Are there any scenarios where non-frontend changes might impact functionality tested as part of the e2e tests?

hmm I'm not 100% sure, but I don't think so because usually any non-frontend changes that impact the frontend will require a corresponding frontend change as well. for example, if we updated the URL for an API or changed the schema, this will usually require updating it in the frontend too. do you have any ideas on non-frontend changes that might have an impact on tested functionality?

@richaagarwal
Copy link
Collaborator

@codemonkey800 Yes, breaking changes to the schema as you've described could be one issue if not coordinated, as well as other unforeseen issues with data validation as we've encountered in the past. It's also possible someone could do a large refactor and accidentally remove or rename endpoints.

Obviously when it's changes we are knowingly introducing we should do our best to ensure frontend and backend are aligned, but accidents/bugs happen & having tests would be one safeguard against this.

I'm curious if this is motivated due to the time it takes for the CI checks to run, or any other concerns with having it run against backend changes as well?

@codemonkey800
Copy link
Collaborator Author

I'm curious if this is motivated due to the time it takes for the CI checks to run, or any other concerns with having it run against backend changes as well?

Mainly because it's a practice we've followed in the past that was changed by #806, and also so that unrelated PRs aren't affected by the frontend E2E tests. It takes maybe 7-10 minutes and are sometimes flaky, which may be confusing to people making non-frontend changes. For example #927 added infra for a new dynamodb table, so running frontend tests seem overkill for such a change

However, I'm fine with keeping it the way that it is for the reasons you listed above too. If folks are okay with the additional CI time for every PR and learn how to rerun flaky tests, then I think it should be fine. cc: @klai95 @manasaV3

@richaagarwal
Copy link
Collaborator

@codemonkey800 Good point on infra changes - maybe we could just run on only frontend and backend dirs. @manasaV3 @klai95 curious for your input as well!

@klai95
Copy link
Contributor

klai95 commented Mar 10, 2023

@codemonkey800 Good point on infra changes - maybe we could just run on only frontend and backend dirs. @manasaV3 @klai95 curious for your input as well!

@codemonkey800 @richaagarwal I second on running on only frontend and backend dirs, as the frontend E2E test do take up quite some time (7-10 minutes), and at times I'd need to rerun them 1-2 times due to flaky tests.

@codemonkey800
Copy link
Collaborator Author

codemonkey800 commented Mar 10, 2023

@richaagarwal @klai95 @manasaV3 running for when backend and frontend dirs sounds like a great idea, but might not be applicable to this workflow. this workflow runs e2e tests against the mock server:

- name: Start mock server on port 8081
run: yarn dev:api &

the new test framework should be capable of testing for staging / prod, but even then, there isn't a GitHub workflow available for it and those tests would be testing against the deployed environments instead of the backend code for a particular PR.

I think what we'll need to do is set up a new workflow that runs the frontend and backend locally together to truly test the application E2E as you suggest @richaagarwal. I've opened up another issue for this as doing this work would be out-of-scope for this PR: #938

eventually we might be able to replace the mock server workflow with the new workflow described above

@richaagarwal
Copy link
Collaborator

ooh thanks for the additional context @codemonkey800 ! I was actually wondering about whether this hits backend code at all, so very good to know. In that case, this PR looks good 👍🏼

@codemonkey800 codemonkey800 merged commit 3ef80eb into main Mar 14, 2023
@codemonkey800 codemonkey800 deleted the jeremy/frontend-e2e-test-filter branch March 14, 2023 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cicd Release Label: Used for categorizing CICD changes in automated CI release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants