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: disable selflink test on apple silicon #7411

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Apr 24, 2024

Arborist CI has started failing on macos-latest now that those runners default to arm64 machines (aka Apple Silicon). I am able to reproduce the failures locally on a Macbook Pro M1.

After spending some time debugging the issue I believe it has to do with the timing of Node vs Link creation. I was able to bisect and find #5376 which removed the ability for nodes to possibly take longer to create than their link targets.

Going back to the commit before that PR the flaky test passes locally for me and fails starting with the first commit in that PR.

I'm just running the offending test in a loop and seeing if it fails, so not a perfect metric. But when it fails, I get a failure at least 10% of the time. On the old commit I was able to run it 50x with no failures. Here's what I was running locally to observe failures:

COUNT="0"

while true; do
  COUNT=$((COUNT+1))
  echo "Start $COUNT"
  if ! npm test -w workspaces/arborist --ignore-scripts -- test/arborist/load-actual.js --no-coverage -Rtap --grep selflink; then
    echo "Failed on run $COUNT"
    exit 1
  fi
done

This is definitely an edge case, but one I would like to fix in the future. Disabling this test is to temporarily get CI green while we release and make more substantial changes that are hard to do with CI flaking.

We've had other issues with symlinks and I would feel much better knowing we have defined behavior in this specific case when tracking down future potential symlink bugs.

One fix that worked locally is iterating over node.target.children sequentially instead of in Promise.all] but that is probably only a side effect of the dep ordering in the test. A fix will have to account for any order of links and node taking different amount of time.

@lukekarrys lukekarrys force-pushed the lk/arborist-selflink-test branch from 4edf804 to c1152e9 Compare April 24, 2024 16:47
@lukekarrys lukekarrys changed the title chore: disable selflink test chore: disable selflink test on apple silicon Apr 24, 2024
@wraithgar
Copy link
Member

Is this really just test flakiness or does this represent a real bug/race condition that we have for folks?

@lukekarrys
Copy link
Contributor Author

I think there is a real bug here. The only thing I'm unsure of is how much of an edge case this is. The flaky test uses a symlink back to itself, which I consider more of an edge case than if all symlinks could hit this race condition.

@lukekarrys lukekarrys merged commit dd39de7 into latest Apr 24, 2024
24 checks passed
@lukekarrys lukekarrys deleted the lk/arborist-selflink-test branch April 24, 2024 17:32
@github-actions github-actions bot mentioned this pull request Apr 24, 2024
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.

2 participants