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

fix(frontend): fix test flakiness #1162

Merged
merged 7 commits into from
Apr 29, 2021
Merged

fix(frontend): fix test flakiness #1162

merged 7 commits into from
Apr 29, 2021

Conversation

zwliew
Copy link
Contributor

@zwliew zwliew commented Apr 28, 2021

We were experiencing flaky tests as described in issue #1161.

Problem

2 issues:

  1. Some tests were failing with an error saying that the test exceeded the timeout of 5000ms.

  2. The integration tests were written under the assumption that REACT_APP_ANNOUNCEMENT_ACTIVE != "true". However, if it is set to true, the integration tests will break due to missing API endpoints and the tests not accounting for the need to dismiss the announcement modal.

Closes #1161

Solution

  • Increase the timeout of each test to 20s. 10s works fine locally on a MBP 16", but given that CI servers are likely slower, 2x the timeout to be safe.
  • Add a dummy API mock for /settings/announcement-version
  • Disable announcements for tests to enforce the assumption that REACT_APP_ANNOUNCEMENT_ACTIVE=false. This is fine because none of our tests rely on announcements.

Note: a longer term fix would be to add support for announcement_version in the API mocks, and configure the initial announcement_version appropriately during each test.

Tests

All CI tests should pass reliably.

Deploy Notes

  • Amplify no longer runs the tests. Any test failures should be caught on Travis during development. If a test failure does get to deployment, we can manually redeploy an older release.

We were experiencing flaky tests as described in issue #1161.

A timeout of 10s works fine on a local MBP 16". However, given that
the CI servers are likely much weaker, 2x the timeout to be on the safe
side.
@zwliew
Copy link
Contributor Author

zwliew commented Apr 28, 2021

To improve test runtimes, we could explore using --runInBand or maxWorkers=4 based on comments here: jestjs/jest#1524 and the Jest docs here: https://jestjs.io/docs/troubleshooting#tests-are-extremely-slow-on-docker-andor-continuous-integration-ci-server

It seems that since Travis only provides 2 cores, and since Jest runs with cpuCount - 1 cores in CI mode, Jest only uses 1 worker by default.

@@ -5,6 +5,8 @@ import { Crypto } from '@peculiar/webcrypto'
import 'locales' // Locales necessary for I18nProvider
import { server } from './test-utils'

jest.setTimeout(20000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can introduce an environment variable with a fallback to a default for this? This way if we want to tune this for different environments, we do not need to make changes in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I'll do that with a default of 20s.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do test this out on Amplify also with repeated runs.

Copy link
Contributor Author

@zwliew zwliew Apr 29, 2021

Choose a reason for hiding this comment

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

Yup, I'm doing so on both Amplify and Travis

This is so that we can configure the timeout based on the test
environment.
We don't have any tests that test the announcement modal.
Hence, disable it for now.

A longer-term fix would be to add announcement_version to the initial
state of the API mock.
@zwliew zwliew changed the title fix(frontend): increase jest timeout to 20s fix(frontend): increase timeout to 20s and disable announcements for tests Apr 29, 2021
@lamkeewei
Copy link
Contributor

lamkeewei commented Apr 29, 2021

Also remember to remove running tests on Amplify as discussed during sync up.

@zwliew zwliew changed the title fix(frontend): increase timeout to 20s and disable announcements for tests fix(frontend): fix frontend test flakiness Apr 29, 2021
We only rely on Amplify for deployments, so there isn't a major reason
to run tests there.

The main reason is to block the deployment if a test fails, but any test
failures should've been caught on Travis during development.

If a failure does happen during deployment, we can manually redeploy an
older release.
@zwliew zwliew marked this pull request as ready for review April 29, 2021 06:47
@zwliew
Copy link
Contributor Author

zwliew commented Apr 29, 2021

I'll continue running tests until deployment tonight, but this should fix the issue that you had locally as well as the timeout issues.

@@ -17,6 +18,9 @@ if (process.env.NODE_ENV === 'test') {
gaInitializeOptions = {
testMode: true,
}

// Disable announcements in test environments
announcementActive = 'false'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about disabling announcements for tests? I think it's fine since none of our tests rely on announcements currently.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine. Given that for most of the flows, that's not the main thing we're testing for.

Copy link
Contributor

@lamkeewei lamkeewei left a comment

Choose a reason for hiding this comment

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

lgtm! This fixed the issues I was facing previously when running tests. Merge whenever you're ready.

@zwliew zwliew changed the title fix(frontend): fix frontend test flakiness fix(frontend): fix test flakiness Apr 29, 2021
@zwliew zwliew merged commit 04c49a2 into develop Apr 29, 2021
@zwliew zwliew deleted the fix-jest-timeout branch April 29, 2021 11:13
lamkeewei added a commit that referenced this pull request May 5, 2021
* develop:
  refactor: use shared function to initialize models (#1172)
  chore: setup scaffolding for backend tests (#940)
  1.23.0
  fix(frontend): fix frontend test flakiness (#1162)
  fix: update successful delivery status only if error does not exist (#1150)
  chore: upgrade dependencies (#1153)
  feat: add unit tests for error states in critical workflows (#1118)
  feat: support whitelisting domains through `agencies` table (#1141)
  feat: add tests for happy paths in critical workflows (#1110)
  fix: prevent campaign names from causing dashboard rows to overflow (#1147)
  fix(email): Fix SendGrid fallback integration (#1026)
lamkeewei added a commit that referenced this pull request May 5, 2021
* develop:
  feat: refactor msg template components; add telegram character limit (#1148)
  refactor: use shared function to initialize models (#1172)
  chore: setup scaffolding for backend tests (#940)
  1.23.0
  fix(frontend): fix frontend test flakiness (#1162)
  fix: update successful delivery status only if error does not exist (#1150)
  chore: upgrade dependencies (#1153)
  feat: add unit tests for error states in critical workflows (#1118)
  feat: support whitelisting domains through `agencies` table (#1141)
  feat: add tests for happy paths in critical workflows (#1110)
  fix: prevent campaign names from causing dashboard rows to overflow (#1147)
lamkeewei added a commit that referenced this pull request May 10, 2021
* develop:
  fix: fix error when updating Telegram ID for an existing phone number (#1178)
  chore: upgrade React; use new JSX transform; sort imports (#1129)
  fix(backend): docker build and tsc output directory structure (#1177)
  feat: refactor msg template components; add telegram character limit (#1148)
  refactor: use shared function to initialize models (#1172)
  chore: setup scaffolding for backend tests (#940)
  1.23.0
  fix(frontend): fix frontend test flakiness (#1162)
  fix: update successful delivery status only if error does not exist (#1150)
  chore: upgrade dependencies (#1153)
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.

Flaky frontend tests
2 participants