test: Avoid race conditions with symlinks #13498
Conversation
Also, no need to double-symlink stuff, and since we now build into unique dirs, we can skip the check for existing symlinks as well.
mydea
force-pushed
the
fn/test-race-conditions
branch
from
df53722
to
44d1d00
Compare
| // ignore errors here, probably means the file already exists | ||
| // Since we always build into a new directory for each test, we can safely ignore this |
There was a problem hiding this comment.
Wondering if we are better off violently crashing, so that we catch this case when it happens...
There was a problem hiding this comment.
the problem is that this does happen :D tests fail without this. the alternative is to check if the file already exists before doing this, but a) this is in reality more overhead than try-catching this (because it only happens in a handful of tests where we have multiple HTML files), and b) it is still prone to flake because of possible race conditions. IMHO it is safe to do this, because each dir that it is building for is for a single test, where we never have the case/desire to have the same file being different from each other - they can safely be the same one always.
* develop: ref: Add external contributor to CHANGELOG.md (getsentry#13500) test: Avoid race conditions with symlinks (getsentry#13498) fix(node): Suppress tracing for transport request execution rather than transport creation (getsentry#13491) ci: Ensure cache save happens after install step (getsentry#13497) test: Increase timeout for redis-cache test & docker-compose (getsentry#13499) Fix config example in README.md for Nuxt (getsentry#13496)
Noticed here: https://github.com/getsentry/sentry-javascript/actions/runs/10594383971/job/29358133582 that this was sometimes failing. While looking into this, we actually did unnecessary work here - we had two levels of symlinks. Now we simply have a single symlink, and since we have unique dirs now we can skip checking for existing files etc.