Skip to content

Setup separate GA jobs for firestore #7523

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

Merged
merged 47 commits into from
Aug 16, 2023
Merged
Changes from 45 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
1774754
Setup firestore tests for named db
wu-hui Jul 31, 2023
1b92147
fix error
wu-hui Aug 2, 2023
2ff3e0f
Fail CI intentionally
wu-hui Aug 2, 2023
05816aa
Fix
wu-hui Aug 2, 2023
00c2ef5
Run named db lite only
wu-hui Aug 2, 2023
29fce8f
browser:prod
wu-hui Aug 2, 2023
48ce3de
try more
wu-hui Aug 2, 2023
5e9e606
more more
wu-hui Aug 2, 2023
37b690e
try sequential
wu-hui Aug 2, 2023
32e9780
both parallelization
wu-hui Aug 2, 2023
558efd7
Better parallelization
wu-hui Aug 2, 2023
8dd9483
more tuning
wu-hui Aug 3, 2023
27d56c4
separate
wu-hui Aug 3, 2023
11dbbfc
sequential
wu-hui Aug 3, 2023
1aeb561
all sequential
wu-hui Aug 3, 2023
0809b3a
Setup separate jobs
wu-hui Aug 3, 2023
500b745
goes further
wu-hui Aug 3, 2023
36112e6
Skip script
wu-hui Aug 4, 2023
1d19498
Fix job failure
wu-hui Aug 4, 2023
2c67e63
no lerna
wu-hui Aug 4, 2023
68c8611
separate runner for compat
wu-hui Aug 4, 2023
63e25ed
better name
wu-hui Aug 4, 2023
ccf3bc6
address flaky tests
wu-hui Aug 4, 2023
0696e93
even less checks
wu-hui Aug 4, 2023
a73cef1
Merge branch 'master' into wuandy/ParallelByGH
wu-hui Aug 4, 2023
12f63d1
Fix and skip flaky tests
wu-hui Aug 14, 2023
03d71ec
Merge branch 'wuandy/ReduceFlakiness' into wuandy/ParallelByGH
wu-hui Aug 14, 2023
55c8ff1
separate build and tests
wu-hui Aug 14, 2023
692850f
add todos
wu-hui Aug 14, 2023
2f3e297
Merge branch 'wuandy/ReduceFlakiness' into wuandy/ParallelByGH
wu-hui Aug 14, 2023
5891d7c
undo script change
wu-hui Aug 14, 2023
f1b7044
more nits
wu-hui Aug 14, 2023
af32139
Merge branch 'wuandy/ReduceFlakiness' into wuandy/ParallelByGH
wu-hui Aug 14, 2023
94bbb00
slight improvements
wu-hui Aug 14, 2023
068179d
fix
wu-hui Aug 14, 2023
74e62e3
yarn yarn
wu-hui Aug 14, 2023
7d9f850
Merge branch 'master' into wuandy/ParallelByGH
wu-hui Aug 14, 2023
76ffe85
changed_firestore
wu-hui Aug 14, 2023
50379f6
no yarn
wu-hui Aug 14, 2023
774a6fa
no node_modules
wu-hui Aug 14, 2023
f6442ef
detect changed
wu-hui Aug 14, 2023
6d86c84
No firestore change
wu-hui Aug 15, 2023
8e7f95c
fix
wu-hui Aug 15, 2023
df87cac
firestore change
wu-hui Aug 15, 2023
46dfa4e
no firestore
wu-hui Aug 15, 2023
d8e476b
empty lines
wu-hui Aug 16, 2023
1e17b96
Add todo
wu-hui Aug 16, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 106 additions & 15 deletions .github/workflows/test-changed-firestore.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,16 @@ name: Test Firestore

on: pull_request

env:
artifactRetentionDays: 14

jobs:
test-chrome:
name: Test Firestore on Chrome and Node If Changed
build:
name: Build Firestore

runs-on: ubuntu-latest
outputs:
changed: ${{ steps.set-output.outputs.CHANGED }}

steps:
- name: Checkout Repo
Expand All @@ -28,27 +34,115 @@ jobs:
cp config/ci.config.json config/project.json
yarn
- name: build
run: yarn build:changed firestore
- name: Run tests if firestore or its dependencies has changed
run: yarn test:changed firestore
id: build
run: yarn build:changed firestore | egrep "Skipping all"
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if there's a build failure that doesn't produce the output Skipping All. Does it still generate an error in steps.build.outcome (which is tested below)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the tests will run and they will fail for missing dependencies because build failed.

Copy link
Contributor

Choose a reason for hiding this comment

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

So if there's a build failure then steps.build.outcome will be success?

Is there a way that we can make this job fail instead, so that the other jobs don't even start? Seems like it would be more clear as to where the problem is when an error occurs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, by piping the output from yarn build:changed firestore through egrep the entire output is lost. So if the build fails there will be no logs indicating why the build failed. Please find a way to do this while preserving the command's output in the logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we could make yarn build:changed > SOME_OUTPUT a separate step, and do the grepping in the next step.

For now, can I get by with a TODO?

Copy link
Contributor

Choose a reason for hiding this comment

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

You'd probably want to redirect stderr as well, so that it shows up inline. Using the tee command seems like a good candidate here:

yarn build:changed 2>&1 | tee ${{ runner.temp }}/yarn.log.txt

This would print the output to stdout and also save it into a temp file. The next line in the step's bash script could run egrep.

# Continue when "Skipping all" is not found
continue-on-error: true
- name: set output
# This means "Skipping all" was not found
if: steps.build.outcome != 'success'
id: set-output
run: echo "CHANGED=true" >> "$GITHUB_OUTPUT";
- name: Archive build
if: ${{ !cancelled() && steps.build.outcome != 'success' }}
run: |
tar -cf build.tar --exclude="\.git" .
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Change --exclude="\.git" to --exclude=.git because you don't have to "escape" the . character. The pattern given to --exclude is a "glob" pattern (see man 7 glob) which does not treat . specially.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will address in the next PR.

gzip build.tar
- name: Upload build archive
if: ${{ !cancelled() && steps.build.outcome != 'success' }}
uses: actions/upload-artifact@v3
with:
name: build.tar.gz
path: build.tar.gz
retention-days: ${{ env.artifactRetentionDays }}

compat-test-chrome:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also add a compat-test-firefox? It looks like the old code ran the compat tests on firefox as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will address in the next PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a reminder to make sure to add compat-test-firefox to this repo's "required" checks when you add it.

name: Test Firestore Compatible

Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty lines please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

runs-on: ubuntu-latest

Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

needs: build
if: ${{ needs.build.outputs.changed == 'true'}}
steps:
- name: Set up Node (14)
uses: actions/setup-node@v3
with:
node-version: 14.x
- name: install Chrome stable
run: |
sudo apt-get update
sudo apt-get install google-chrome-stable
- name: Download build archive
uses: actions/download-artifact@v3
with:
name: build.tar.gz
- name: Unzip build artifact
run: tar xf build.tar.gz
- name: Bump Node memory limit
run: echo "NODE_OPTIONS=--max_old_space_size=4096" >> $GITHUB_ENV
- name: Test setup and yarn install
run: |
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: reduce this single-line bash script to one line (i.e. get rid of the pipe | and bring the line below up to this line). Also on lines 113 and 148.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will address in the next PR.

cp config/ci.config.json config/project.json
- name: Run compat tests
run: cd packages/firestore-compat && yarn run test:ci

test-chrome:
name: Test Firestore
strategy:
matrix:
test-name: ["test:browser", "test:travis", "test:lite:browser", "test:browser:prod:nameddb", "test:lite:browser:nameddb"]

runs-on: ubuntu-latest

needs: build
if: ${{ needs.build.outputs.changed == 'true'}}
steps:
- name: Set up Node (14)
uses: actions/setup-node@v3
with:
node-version: 14.x
- name: install Chrome stable
run: |
sudo apt-get update
sudo apt-get install google-chrome-stable
- name: Download build archive
uses: actions/download-artifact@v3
with:
name: build.tar.gz
- name: Unzip build artifact
run: tar xf build.tar.gz
- name: Bump Node memory limit
run: echo "NODE_OPTIONS=--max_old_space_size=4096" >> $GITHUB_ENV
- name: Test setup and yarn install
run: |
cp config/ci.config.json config/project.json
- name: Run tests
run: cd packages/firestore && yarn run ${{ matrix.test-name }}


test-firefox:
name: Test Firestore on Firefox If Changed
name: Test Firestore on Firefox
strategy:
matrix:
test-name: ["test:browser", "test:travis", "test:lite:browser", "test:browser:prod:nameddb", "test:lite:browser:nameddb"]
# Whatever version of Firefox comes with 22.04 is causing Firefox
# startup to hang when launched by karma. Need to look further into
# why.
runs-on: ubuntu-20.04

needs: build
if: ${{ needs.build.outputs.changed == 'true'}}
steps:
- name: install Firefox stable
run: |
sudo apt-get update
sudo apt-get install firefox
- name: Checkout Repo
uses: actions/checkout@master
- name: Download build archive
uses: actions/download-artifact@v3
with:
# This makes Actions fetch all Git history so run-changed script can diff properly.
fetch-depth: 0
name: build.tar.gz
- name: Unzip build artifact
run: tar xf build.tar.gz
- name: Set up Node (14)
uses: actions/setup-node@v3
with:
Expand All @@ -58,10 +152,7 @@ jobs:
- name: Test setup and yarn install
run: |
cp config/ci.config.json config/project.json
yarn
- name: build
run: yarn build:changed firestore
- name: Run tests if firestore or its dependencies has changed
run: xvfb-run yarn test:changed firestore
- name: Run tests
run: cd packages/firestore && xvfb-run yarn run ${{ matrix.test-name }}
env:
BROWSERS: 'Firefox'