test(e2e-helper): verify + retry remote-scope registration in capsule builds#10485
Open
davidfirst wants to merge 2 commits into
Open
test(e2e-helper): verify + retry remote-scope registration in capsule builds#10485davidfirst wants to merge 2 commits into
davidfirst wants to merge 2 commits into
Conversation
…capsule-build flakiness
PR Summary by QodoHarden e2e remote-scope registration with verification + bounded retry
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
…fying remote registration
Comment on lines
+133
to
+138
| // eslint-disable-next-line no-console | ||
| console.log( | ||
| `[e2e-helper] WARNING: remote "file://${remoteScopePath}" is not registered in the workspace ` + | ||
| `scope.json at "${cwd}" after ${maxRetries + 1} attempts. remotes on disk: ` + | ||
| JSON.stringify(this.readWorkspaceRemotes(cwd)) | ||
| ); |
There was a problem hiding this comment.
1. Raw console.log warnings added 📘 Rule violation ⚙ Maintainability
New warning output is emitted via direct console.log with hardcoded text, rather than using the repository’s shared CLI output formatter/style guide. This can lead to inconsistent CLI/test-run output and makes future output changes harder to standardize.
Agent Prompt
## Issue description
New warning messages are printed with direct `console.log` and hardcoded formatting, instead of using the shared CLI output formatter per the repository style guide.
## Issue Context
This PR introduces new diagnostic output in test infrastructure (`e2e-helper` and `mockWorkspace`). Even if intended for CI diagnostics, the repository compliance rule requires output changes to follow the shared formatting toolkit and style guide to keep output consistent.
## Fix Focus Areas
- components/legacy/e2e-helper/e2e-scope-helper.ts[133-138]
- scopes/workspace/testing/mock-workspace/mock-workspace.ts[28-31]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+160
to
+168
| private isRemoteRegistered(remoteScopePath: string, cwd: string): boolean { | ||
| const scopeName = path.basename(remoteScopePath); | ||
| const remotes = this.readWorkspaceRemotes(cwd); | ||
| // match by scope name (the registered key) or by the host value referencing the remote path — | ||
| // basename is stable against any path normalization the remote-add may apply to the host. | ||
| return ( | ||
| scopeName in remotes || | ||
| Object.values(remotes).some((host) => typeof host === 'string' && host.includes(scopeName)) | ||
| ); |
There was a problem hiding this comment.
2. Remote check too permissive 🐞 Bug ☼ Reliability
ScopeHelper.isRemoteRegistered() treats a remote as registered if any existing remote host string merely contains the new remote directory basename, which can match unrelated remotes and incorrectly suppress the retry/warning path. This reduces the effectiveness/diagnostic value of the new hardening in flaky CI runs.
Agent Prompt
## Issue description
`isRemoteRegistered()` currently returns true when **any** configured remote host string `includes(scopeName)`. This can yield false positives (substring matches) and skip the retry/warning branch even when the intended remote wasn’t actually registered.
## Issue Context
`bit remote add` persists remotes under a deterministic key (`remote.name`) and value (`remote.host`). The verification can be made precise by checking the expected key and/or normalizing the configured host into a path/URL and comparing it to the intended `remoteScopePath`.
## Fix Focus Areas
- components/legacy/e2e-helper/e2e-scope-helper.ts[160-168]
### Suggested fix approach
- Prefer verifying by **key** (remote scope name) and **exact host** rather than substring:
- Determine the expected remote name:
- Option A (best): read `${remoteScopePath}/scope.json` and use its `name` field.
- Option B: keep `path.basename(remoteScopePath)` but then do *not* use substring matching.
- Determine the expected host for file remotes:
- Compare against the exact URL you pass to `bit remote add` (or normalize both to filesystem paths using `fileURLToPath` when the host is a `file://` URL).
- Replace `host.includes(scopeName)` with either:
- an exact key check (`expectedName in remotes`), and/or
- a precise host comparison for that key (`remotes[expectedName] === expectedHost` or normalized path equality).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit b5f8bf4 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Integration specs that create file-based remote scopes (snapping/checkout/lanes via
mockWorkspace) intermittently fail only underbit ci pr --build(capsule builds), not underbit test, with:InvalidScopeNameFromRemote: cannot find scope '<hash>-remote'MissingBitMapComponent: component "<hash>-remote/comp1" was not foundRoot cause: these in-process integration tests share global state with no per-test isolation (the global config/scope dir is frozen at module-load from
~/Library/Caches/Bit). Under a build, dozens of them run back-to-back in one long-lived process alongside the host build machinery, so a just-registered<hash>-remoteoccasionally isn't resolvable to a later import, which then falls through to the bit.cloud hub.This change
Hardens the e2e-helper remote registration as a low-risk first step:
ScopeHelper.addRemoteScope()now verifies the remote actually landed in the workspacescope.jsonand re-adds it (bounded retry) if not — recovering a transient write race. If it still can't be verified it logs a loud, diagnosable message (does not throw), so the hundreds of other callers can't regress and any residual CI failure clearly points at registration vs. an import-side resolution problem.mockWorkspace({ bareScopeName })warns clearly if the reconstructed bare-scope path doesn't exist, instead of surfacing a cryptic scope-resolution error later.Test-infra only; no product behavior change. This is deliberately conservative — we'll watch several
bit_prruns to see whether it removes the flake or localizes it to the import side for a follow-up.