Remove Jest test matrix

[puneetlath]

There are a number of problems with the current Jest testing workflow:

1. It has been known to sometimes skip certain tests (example)
2. The file itself is confusing / more complicated than necessary
3. Any errors that occur in jest are masked by the 123 exit code of xargs

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0172a4a7d9f592a3ac

[melvin-bot]

Current assignee @kevinksullivan is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (External)

[melvin-bot]

Triggered auto assignment to @thienlnam (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[stavares843]

proposal

name: Jest Unit Tests

on:
  workflow_call:
  pull_request:
    types: [opened, synchronize]
    branches-ignore: [staging, production]

jobs:
  test:
    if: ${{ github.actor != 'OSBotify' || github.event_name == 'workflow_call' }}
    runs-on: ubuntu-latest
    steps:
      - uses: Expensify/App/.github/actions/composite/setupNode@main

      # If automatic signing is enabled, iOS builds will fail, so ensure we always have the proper profile specified
      - name: Check Provisioning Style
        run: |
          if grep -q 'PROVISIONING_PROFILE_SPECIFIER = chat_expensify_appstore' ios/NewExpensify.xcodeproj/project.pbxproj; then
            exit 0
          else
            echo "Error: Automatic provisioning style is not allowed!"
            exit 1
          fi
      - name: Jest Unit Tests
        run: npm run test
        env:
          CI: true

      - name: Pull Request Tests
        run: tests/unit/getPullRequestsMergedBetweenTest.sh

      - name: Cache Jest cache
        id: cache-jest-cache
        uses: actions/cache@v1
        with:
          path: .jest-cache
          key: ${{ runner.os }}-jest

      - name: Pull Request Tests
        # Pull request related tests will be run in separate runner in parallel.
        run: tests/unit/getPullRequestsMergedBetweenTest.sh

[Prince-Mendiratta]

Proposal

We can remove the matrix. I noticed @roryabraham added a method to get the number of CPU cores and assign maximum workers accordingly, "because it seemed like there might’ve been zombie threads sitting around never finishing even though all tests had passed", as noted in this comment. If that is an intended feature to implement, we can add that.

A sample run with this action can be found at - https://github.com/Prince-Mendiratta/expensify-app/actions/runs/3859244680.

Also, why are we not using actions/cache@v3? I tried it and it works well. I propose that we use the latest version of cache, further considering that save-state is being deprecated and will stop working on June 1, 2023 and cache@v1 makes use of save-state.

Is there a specific reason why we do not cache npm modules as well? We could cache npm modules and if the hash of the package.json changes, we can reinstall npm modules. This would further cut down a huge amount of time.

diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 72dde5fe38..72099e68c9 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -6,37 +6,17 @@ on:
     types: [opened, synchronize]
     branches-ignore: [staging, production]
 
-env:
-  # Number of parallel jobs for jest tests
-  CHUNKS: 3
 jobs:
-  config:
-    runs-on: ubuntu-latest
-    name: Define matrix parameters
-    outputs:
-      MATRIX: ${{ steps.set-matrix.outputs.MATRIX }}
-      JEST_CHUNKS: ${{ steps.set-matrix.outputs.JEST_CHUNKS }}
-    steps:
-      - name: Set Matrix
-        id: set-matrix
-        uses: actions/github-script@v6
-        with:
-          # Generate matrix array i.e. [0, 1, 2, ...., CHUNKS - 1] for test job
-          script: |
-            core.setOutput('MATRIX', Array.from({ length: Number(process.env.CHUNKS) }, (v, i) => i + 1));
-            core.setOutput('JEST_CHUNKS', Number(process.env.CHUNKS) - 1);
-
   test:
-    needs: config
     if: ${{ github.actor != 'OSBotify' || github.event_name == 'workflow_call' }}
     runs-on: ubuntu-latest
-    name: test (job ${{ fromJSON(matrix.chunk) }})
+    name: test (shard ${{ fromJSON(matrix.chunk) }})
     env:
       CI: true
     strategy:
       fail-fast: false
       matrix:
-        chunk: ${{fromJson(needs.config.outputs.MATRIX)}}
+        chunk: [1, 2, 3]
 
     steps:
       # This action checks-out the repository, so the workflow can access it.
@@ -55,7 +35,11 @@ jobs:
             echo "Error: Automatic provisioning style is not allowed!"
             exit 1
           fi
-
+      
+      - name: Get number of CPU cores
+        id: cpu-cores
+        uses: SimenB/github-actions-cpu-cores@31e91de0f8654375a21e8e83078be625380e2b18
+      
       - name: Cache Jest cache
         id: cache-jest-cache
         uses: actions/cache@v1
@@ -64,11 +48,9 @@ jobs:
           key: ${{ runner.os }}-jest
 
       - name: All Unit Tests
-        if: ${{ fromJSON(matrix.chunk) < fromJSON(env.CHUNKS) }}
         # Split the jest based test files in multiple chunks/groups and then execute them in parallel in different jobs/runners.
-        run: npx jest --listTests --json | jq -cM '[_nwise(length / ${{ fromJSON(needs.config.outputs.JEST_CHUNKS) }} | ceil)]' | jq '[[]] + .' | jq '.[${{ fromJSON(matrix.chunk) }}] | .[] | @text' | xargs npm test
+        run: npx jest --shard=${{ fromJSON(matrix.chunk) }}/${{ strategy.job-total }} --max-workers ${{ steps.cpu-cores.outputs.count }}
 
       - name: Pull Request Tests
         # Pull request related tests will be run in separate runner in parallel.
-        if: ${{ fromJSON(matrix.chunk) == fromJSON(env.CHUNKS) }}
         run: tests/unit/getPullRequestsMergedBetweenTest.sh

[roryabraham]

sorry for the lack of communication on my part here. I have a draft PR which reimplements the matrix in a much simpler way using Jest's --shard feature. I'm partial to this approach because it resolves most of my concerns with the matrix approach. #13943

[Prince-Mendiratta]

Updated Proposal

I've been working and testing different kinds of configurations and have added my understandings through my tests. We can choose either of these workflows depending on what we're looking for.

Background

In the current implementation, the OP of the PR manually divided the jest tests into chunks and then executed them as batches/groups. This is a approach that was used in lower versions of jest, where the shard argument was not supported and tests had to be divided manually. However, the version that we using with Expensify support the use of the shard argument, using which jest automatically constructs the batches in an intelligent and non complex manner. Thus, we can directly use the matrix feature of Github actions along with the shard argument in jest to divide tests in an easy to debug and execute manner.

Testing workflow

I've forked the repo and create new tests with different configurations. I then created a PR to the repo through another account, commiting just text files to run the Github Actions. Then, I analyse how the results differ. The two major contributing factors to the time are-

1. Installing node dependencies
2. Running jest tests

Both of them take around 1:30 - 3:00 minutes each, depending on the optimization.
With the use of jest cache, the time for jest tests can be reduced to less than 1 minute, per shard.

Base

• No sharding, single runner.
• .npm with cache and npm ci executed.
• Directly executes npm test
• No caching, other than .npm cache from setupNode
• Takes around 3-6 minutes, each time. Each major task of the two is completed in 1:30-2:30 minutes.

Workflow log - https://github.com/Prince-Mendiratta/expensify-app/actions/workflows/test.yml
Config file - https://github.com/Prince-Mendiratta/expensify-app/blob/main/.github/workflows/test.yml
Results:

Jest Optimisation

• 3 shards.
• .npm with cache and npm ci executed.
• Uses the --shard and --max-workers config in jest
• Uses jest cache.
• Cache miss takes up around 5 minutes to complete from scratch
• Takes 2:50-4:00 minutes to complete.

Workflow log - https://github.com/Prince-Mendiratta/expensify-app/actions/workflows/test2.yml
Config file - https://github.com/Prince-Mendiratta/expensify-app/blob/main/.github/workflows/test2.yml
Results:

2 shards, Jest optimised

• 2 shards.
• .npm with cache and npm ci executed.
• Uses the --shard and --max-workers config in jest
• Uses jest cache.
• Cache miss takes up around 5 minutes to complete from scratch
• Takes 3:30-4:30 minutes to complete.

Workflow log - https://github.com/Prince-Mendiratta/expensify-app/actions/workflows/test5.yml
Config file - https://github.com/Prince-Mendiratta/expensify-app/blob/main/.github/workflows/test5.yml
Results:

3 shards, cached node_modules

• 3 shards.
• Uses the --shard and --max-workers config in jest
• Does not use jest cache.
• Caches node_modules based on the hash of package-lock.json, inducing cache miss and reinstalling dependencies if there have been any changes to the package-json.lock file
• If node_modules cache hit, skips installing node_modules and reuses cache.
• If cache does not exist for the hash of that particular package-lock.json, reinstalls node_modules with npm ci
• Cache miss takes up around 5 minutes to complete from scratch
• Takes 3 minutes consistently to complete with the tests.

Workflow log - https://github.com/Prince-Mendiratta/expensify-app/actions/workflows/test6.yml
Config file - https://github.com/Prince-Mendiratta/expensify-app/blob/main/.github/workflows/test6.yml
Results:

3 shards, cached node_modules, jest cached

• 3 shards.
• Uses the --shard and --max-workers config in jest
• Caches jest.
• Caches node_modules based on the hash of package-lock.json, inducing cache miss and reinstalling dependencies if there have been any changes to the package-json.lock file
• If node_modules cache hit, skips installing node_modules and reuses cache.
• If cache does not exist for the hash of that particular package-lock.json, reinstalls node_modules with npm ci
• Cache miss takes up around 5 minutes to complete from scratch.
• Takes 1:50-2:30 minutes consistently to complete with the tests.

Workflow log - https://github.com/Prince-Mendiratta/expensify-app/actions/workflows/test7.yml
Config file - https://github.com/Prince-Mendiratta/expensify-app/blob/main/.github/workflows/test7.yml
Results:

Side - by - side comparison

Overall, I believe that with the use of cache, we can reduce the time highly whilst still maintaining the simplicity with either of my solutions. We can further improve times to under 1 minute by using large-runner by increasing the number of cores, we can increase the number of shards as well as maxworkers per shard, optimizing the speed by a huge amount. However, that is something that needs to be discussed properly first since the maintenance of such an implementation will require proper planning.

[roryabraham]

discussed 1:1 with @thienlnam and I'll take this over as I have more context.

[kevinksullivan]

I'll go ahead and unassign @Santhosh-Sellavel and I, but let me know if I am missing something here @roryabraham .

[Prince-Mendiratta]

@roryabraham Just wondering, is it a bad idea to cache node_modules as well as suggested in my proposal, considering the use case for Expensify?

[melvin-bot]

@roryabraham Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@roryabraham Eep! 4 days overdue now. Issues have feelings too...

[MelvinBot]

This issue has not been updated in over 14 days. @roryabraham eroding to Weekly issue.

[MelvinBot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @mananjadhav (Internal)

[roryabraham]

PR ready for review: #13943

[MelvinBot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[roryabraham]

No payments due here, so we're done