chore: refactor functional api tests to rely on fs lock#2567
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a filesystem-based cross-process lock and retry policies, a faucet-backed topUpWallet utility used by many functional tests, centralizes mnemonic setup in the custom Jest environment, simplifies TestWalletService to memoized persistent mnemonic storage, and wires topUpWallet into multiple test suites' beforeAll hooks. Changes
Sequence Diagram(s)sequenceDiagram
actor Test as Functional Test
participant WalletUtil as topUpWallet()
participant FsLock as FS Lock File
participant BalanceSvc as Node Balance
participant FaucetSvc as Faucet Service
participant Waiter as Balance Wait Policy
Test->>WalletUtil: Call topUpWallet(options)
WalletUtil->>BalanceSvc: Get current balance
BalanceSvc-->>WalletUtil: Current balance
alt balance < minAmount
WalletUtil->>FsLock: Acquire faucet lock (withFaucetLock)
FsLock-->>WalletUtil: Lock acquired
WalletUtil->>BalanceSvc: Re-check balance
BalanceSvc-->>WalletUtil: Current balance (still low)
WalletUtil->>FaucetSvc: POST top-up (faucetRetryPolicy)
FaucetSvc-->>WalletUtil: 2xx or retry/errors
WalletUtil->>Waiter: Poll balance (balanceWaitPolicy)
Waiter->>BalanceSvc: Repeated checks
BalanceSvc-->>Waiter: Updated balance
Waiter-->>WalletUtil: Balance >= minAmount
WalletUtil->>FsLock: Release faucet lock
FsLock-->>WalletUtil: Lock removed
else balance >= minAmount
WalletUtil-->>Test: Return current balance (no-op)
end
WalletUtil-->>Test: Return final balance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/api/test/services/fs-lock.ts`:
- Around line 26-155: The current logic in withFsLock may delete a live lock if
staleLockThreshold is shorter than the maximum expected critical-section
duration; validate and enforce a safe threshold at start of withFsLock by
checking the merged options (DEFAULT_OPTIONS + opts) and either: 1) throw a
clear Error if options.staleLockThreshold < options.timeout, or 2) automatically
set options.staleLockThreshold = Math.max(options.staleLockThreshold,
options.timeout) (pick one approach consistently), and document this behavior;
update references to FsLockOptions, DEFAULT_OPTIONS, withFsLock, isLockStale,
and tryDeleteStaleLock so stale checks/resets use the validated/adjusted value
and consider adding a comment suggesting a heartbeat alternative for
long-running critical sections.
In `@apps/api/test/services/test-wallet.service.ts`:
- Around line 28-35: getStoredMnemonic is race-prone because it does a
check-then-await on generateMnemonic; memoize the in-flight promise so
concurrent callers share the same generation and avoid duplicate saves.
Implement a companion map (e.g., mnemonicPromises) keyed by fileName; in
getStoredMnemonic, if mnemonics[fileName] exists return it, else if
mnemonicPromises[fileName] exists await and return its resolved value, otherwise
set mnemonicPromises[fileName] = generateMnemonic(), await it, assign the
resolved string to mnemonics[fileName], delete mnemonicPromises[fileName], call
saveCache() once, and return the mnemonic; use the existing function names
getStoredMnemonic, generateMnemonic, mnemonics, and saveCache to locate and
update the code.
🧹 Nitpick comments (2)
apps/api/test/services/topUpWallet.ts (2)
11-11: Consider handling missing config gracefully.The
configvariable uses non-null assertion (config!) at lines 21 and 46. If the.env.functional.testfile is missing or malformed, this will cause a runtime error with an unhelpful message.Optional: Add validation
const { parsed: config } = dotenvExpand.expand(dotenv.config({ path: "env/.env.functional.test" })); + +if (!config) { + throw new Error("Failed to load env/.env.functional.test - ensure the file exists"); +}This would provide a clearer error message if the config file is missing.
81-84: Consider usingresponse.okfor cleaner status check.The current logic is correct but
response.okis more idiomatic for checking successful HTTP responses.Suggested simplification
- if (response.status >= 300 || response.status < 200) { + if (!response.ok) { const errorBody = await response.text().catch(() => "Unknown error"); throw new Error(`Faucet request failed with status ${response.status}: ${errorBody}`); }
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (79.55%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2567 +/- ##
==========================================
- Coverage 50.94% 50.64% -0.30%
==========================================
Files 1069 1059 -10
Lines 29815 29466 -349
Branches 6598 6540 -58
==========================================
- Hits 15188 14923 -265
+ Misses 14344 14147 -197
- Partials 283 396 +113
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
stalniy
force-pushed
the
chore/api-functional-tests
branch
from
900609d to
1843628
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/api/test/services/topUpWallet.ts`:
- Around line 11-22: The code uses a non-null assertion on config when
constructing BalanceHttpService (const { parsed: config } =
dotenvExpand.expand(dotenv.config(...)); new BalanceHttpService({ baseURL:
config!REST_API_NODE_URL })), which can throw cryptic runtime errors if
env/.env.functional.test is missing or malformed; replace the assertion with an
explicit check: after calling dotenvExpand.expand(dotenv.config(...)) validate
that parsed (config) is defined and that config.REST_API_NODE_URL exists, and if
not throw a clear Error (or call process.exit with a descriptive message) before
creating BalanceHttpService so BalanceHttpService always receives a valid
baseURL.
🧹 Nitpick comments (2)
apps/api/test/services/test-wallet.service.ts (1)
35-37: Minor: Redundant nullish coalescing assignment.The
??=on line 36 is unnecessary since we're already inside theif (!this.mnemonics[fileName])block. Consider simplifying:if (!this.mnemonics[fileName]) { - this.mnemonics[fileName] ??= await this.generateMnemonic(); + this.mnemonics[fileName] = await this.generateMnemonic(); this.saveCache(); }apps/api/test/services/fs-lock.ts (1)
189-189: Consider usingpath.joinfor cross-platform consistency.
FAUCET_LOCK_PATHuses template literal string concatenation. For better cross-platform compatibility, consider usingpath.join:-export const FAUCET_LOCK_PATH = `${os.tmpdir()}/cosmos-faucet.lock`; +export const FAUCET_LOCK_PATH = path.join(os.tmpdir(), "cosmos-faucet.lock");
baktun14
previously approved these changes
Jan 26, 2026
stalniy
force-pushed
the
chore/api-functional-tests
branch
from
1843628 to
34ebb57
Compare
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
2 participants
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.
Why
We currently pre-topup test wallets in Jest global setup / custom environment. While this avoids faucet contention, it significantly hurts developer experience:
What
This PR replaces eager, global topups with a lazy funding model:
This approach does not aim to make faucet-dependent tests faster, but it greatly improves DX and test isolation by removing unnecessary global side effects and allowing developers to quickly run or filter tests without paying the faucet cost upfront. However it potentially can speed up functional tests execution because we don't need to wait for redundant wallet creation and topups.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.