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

chore: retry failed Circle CI tests #2814

Merged
merged 13 commits into from
May 3, 2022
79 changes: 63 additions & 16 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,55 @@ executors:

# Custom commands definition
commands:
# Based on https://circleci.com/developer/orbs/orb/kimh/run-with-retry
retry_karma:
description: Retry command multiple times until it succeeds
parameters:
command:
type: string
command_name:
type: string
working_directory:
type: string
retry_count:
type: integer
default: 3
coverage:
type: boolean
default: false
compat:
type: boolean
default: false
disable_synthetic:
type: boolean
default: false
force_native_shadow_mode:
type: boolean
default: false
Comment on lines +39 to +50
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: It feels strange that the generic retry_until_success command accepts coverage, compat, disable_synthetic, and force_native_shadow_mode as parameters since those parameters are only applicable to the Karma tests.

IMO it would be preferable to keep them inside the Kama command. It appears the CircleCi config syntax doesn't accept passing arbitrary objects. Many passing them as via a generic environment parameter would do the trick.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I considered using environment, but it's tricky because right now we are relying on the presence or absence of environment variables. E.g. I'm not sure if passing COVERAGE=0 will actually disable coverage – I suspect it would be converted to a string "0" in JavaScript, which is truthy, so coverage would be enabled. We would have to audit each of our environment variables to make sure they handle "0", "false", etc.

I agree the name retry_until_success is not great. Maybe retry_karma_until_success? Or just retry_karma?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe retry_karma_until_success? Or just retry_karma?

retry_karma sounds good.

We would have to audit each of our environment variables to make sure they handle "0", "false", etc.

You can set those environment variables to a nullish value like an empty string, which would coerce to a false boolean.

COVERAGE="" COMPAT="" yarn test

While looking into this, I learned that bash allows you to define environment variables without any value. The following code would also work.

COVERAGE= COMPAT= yarn test

Again it's a nitpick.

Copy link
Collaborator Author

@nolanlawson nolanlawson May 3, 2022

Choose a reason for hiding this comment

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

Renamed to retry_karma. I think I'd like to keep the existing parameter system, because it's a little unclear to me from reading the Circle CI docs how to pass an environment variable to a command. If there's a good way to refactor this, let's do it in a follow-up PR. 🙂

steps:
- run:
name: << parameters.command_name >>
working_directory: << parameters.working_directory >>
command: |
retry() {
MAX_RETRY=<< parameters.retry_count >>
n=0
until [ $n -ge $MAX_RETRY ]
do
echo "Try $[$n+1]/$MAX_RETRY..."
"$@" && break
n=$[$n+1]
done
if [ $n -ge $MAX_RETRY ]; then
echo "Failed: ${@}" >&2
exit 1
fi
}
<<# parameters.disable_synthetic >> DISABLE_SYNTHETIC=1 <</ parameters.disable_synthetic >> \
<<# parameters.force_native_shadow_mode >> FORCE_NATIVE_SHADOW_MODE_FOR_TEST=1 <</ parameters.force_native_shadow_mode >> \
<<# parameters.compat >> COMPAT=1 <</ parameters.compat >> \
<<# parameters.coverage >> COVERAGE=1 <</ parameters.coverage >> \
retry << parameters.command >>
# Setup
restore_yarn_cache:
description: Restore Yarn cache from previous build
Expand Down Expand Up @@ -93,15 +142,14 @@ commands:
type: boolean
default: true
steps:
- run:
name: Run karma integration tests
- retry_karma:
command_name: Run karma integration tests
working_directory: packages/integration-karma
command: >
<<# parameters.disable_synthetic >> DISABLE_SYNTHETIC=1 <</ parameters.disable_synthetic >>
<<# parameters.force_native_shadow_mode >> FORCE_NATIVE_SHADOW_MODE_FOR_TEST=1 <</ parameters.force_native_shadow_mode >>
<<# parameters.compat >> COMPAT=1 <</ parameters.compat >>
<<# parameters.coverage >> COVERAGE=1 <</ parameters.coverage >>
yarn sauce
disable_synthetic: << parameters.disable_synthetic >>
force_native_shadow_mode: << parameters.force_native_shadow_mode >>
compat: << parameters.compat >>
coverage: << parameters.coverage >>
command: yarn sauce


# Jobs definition
Expand Down Expand Up @@ -162,12 +210,11 @@ jobs:
disable_synthetic: false
compat: false
force_native_shadow_mode: true
- run:
name: Run karma hydration tests
- retry_karma:
command_name: Run karma hydration tests
command: yarn hydration:sauce
working_directory: packages/integration-karma
environment:
COVERAGE: 1
coverage: true
- run:
name: Compute karma coverage
command: yarn coverage
Expand All @@ -181,8 +228,8 @@ jobs:
steps:
- load_workspace
- start_sauce_connect
- run:
name: Run integration test - Chrome SauceLabs
- retry_karma:
command_name: Run integration test - Chrome SauceLabs
command: yarn sauce:prod --browsers chrome
working_directory: packages/integration-tests

Expand All @@ -194,8 +241,8 @@ jobs:
steps:
- load_workspace
- start_sauce_connect
- run:
name: Run integration test - IE11 SauceLabs
- retry_karma:
command_name: Run integration test - IE11 SauceLabs
command: yarn sauce:prod_compat --browsers ie11
working_directory: packages/integration-tests

Expand Down