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

test: set cacheRoot on checksum download #1540

Merged
merged 1 commit into from
Aug 1, 2023
Merged

Conversation

erickzhao
Copy link
Member

@erickzhao erickzhao commented Jul 31, 2023

Checklist

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summary

Fixes #1421.

Our test-linux suite has had a very poor 52% success rate according to CircleCI insights. Each one of the Linux test suites seems to fail about 1/5 of the time.

Job Name Total Credits P95 Duration (seconds) Runs Success Rate
"test-linux-14" 517 227 17 0.82
"test-linux-16" 539 220 19 0.79
"test-linux-18" 542 271 16 0.81
"test-mac" 3031 329 15 1
"test-windows" 1974 272 17 0.88

The observed error was always the following:

Downloading Electron v1.4.13 before running tests...
Error: dest already exists.
    at /home/circleci/project/node_modules/@electron/get/node_modules/fs-extra/lib/move/move.js:41:31
    at /home/circleci/project/node_modules/@electron/get/node_modules/universalify/index.js:23:46

Exited with code exit status 1

This error occurred in our test setup code, which downloads Electron with @electron/get before running the test suite. During this setup process, we download the checksum first as of commit 621560b.

To investigate, I added the DEBUG=@electron/get* environment variable in CI to get verbose debug logging. Two things seem to be occurring:

  1. The initial checksum download is set without a cache folder, so it never gets written to the cache after being downloaded.
Downloading Electron v1.4.13 before running tests...
  @electron/get:index Checking the cache (undefined) for SHASUMS256.txt (https://github.com/electron/electron/releases/download/v1.4.13/SHASUMS256.txt) +0ms
  @electron/get:index Cache miss +3ms
  1. The setup process downloads the SHASUM256.txt checksums with cache misses for each binary. Since these downloads are done in parallel, the filesystem operations run into a race condition:
  @electron/get:cache Moving /tmp/electron-download-gaC39r/SHASUMS256.txt to /home/circleci/.electron/4660c2617dcac48c05988e181e4f7ff85132cd278bba79561816d3e7da1cc50c/SHASUMS256.txt +12ms
  @electron/get:cache * Replacing existing file +0ms
  @electron/get:cache Moving /tmp/electron-download-mSMl8f/SHASUMS256.txt to /home/circleci/.electron/4660c2617dcac48c05988e181e4f7ff85132cd278bba79561816d3e7da1cc50c/SHASUMS256.txt +7ms
  @electron/get:cache Moving /tmp/electron-download-oB433U/SHASUMS256.txt to /home/circleci/.electron/4660c2617dcac48c05988e181e4f7ff85132cd278bba79561816d3e7da1cc50c/SHASUMS256.txt +12ms
  @electron/get:cache * Replacing existing file +1ms
  @electron/get:cache Moving /tmp/electron-download-SeHepQ/SHASUMS256.txt to /home/circleci/.electron/4660c2617dcac48c05988e181e4f7ff85132cd278bba79561816d3e7da1cc50c/SHASUMS256.txt +18ms
Error: dest already exists.
    at /home/circleci/project/node_modules/@electron/get/node_modules/fs-extra/lib/move/move.js:41:31
    at /home/circleci/project/node_modules/@electron/get/node_modules/universalify/index.js:23:46

Fix

To fix this, I set a cacheRoot on the initial SHASUM256.txt download so that we get cache hits when downloading the checksums during the binary download. I'm assuming this is the right fix since we must have pre-downloaded the checksums for a reason.

Evaluation

I got CI to pass 10x in a row! With the 52% success rate, this had a 0.145% chance of happening.

image

@erickzhao erickzhao changed the title debug ci: debug Jul 31, 2023
@erickzhao erickzhao marked this pull request as ready for review July 31, 2023 21:48
@erickzhao erickzhao requested a review from a team as a code owner July 31, 2023 21:48
@erickzhao erickzhao marked this pull request as draft July 31, 2023 21:51
@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Merging #1540 (628fedb) into main (6430d3f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1540   +/-   ##
=======================================
  Coverage   95.58%   95.58%           
=======================================
  Files          15       15           
  Lines         793      793           
=======================================
  Hits          758      758           
  Misses         35       35           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@erickzhao erickzhao changed the title ci: debug test: set cacheRoot on checksum download Aug 1, 2023
@erickzhao erickzhao marked this pull request as ready for review August 1, 2023 01:07
Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

Seems good

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.

ci: dest already exists flake
2 participants