chore(test): fix data verification test #285
Conversation
We need to index the bundle transaction before we can attempting to verify its data root.
| @@ -160,7 +160,7 @@ export class DataRootComputer { | |||
| }); | |||
| } | |||
|
|
|||
| async computeDataRoot(id: string): Promise<Uint8Array | undefined> { | |||
| async computeDataRoot(id: string): Promise<string | undefined> { | |||
There was a problem hiding this comment.
@hlolli - the result of this is the toB64Url of the data root, lmk if i'm mistaken on this change. it was causing type warnings in the verifyDataRoot when trying to compare a string to Uint8Array
There was a problem hiding this comment.
that is correct, initially it was that, but after my changes it's a string
|
Warning Rate limit exceeded@dtfiedler has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request introduces configuration and improvements to the background data verification process. A new configuration constant Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration
participant Worker as DataVerificationWorker
participant DataRoot as DataRootComputer
Config->>Worker: Set worker count
Worker->>DataRoot: Compute data root
alt Data root exists
DataRoot-->>Worker: Return Base64 URL string
Worker->>Worker: Verify data root
else Data root undefined
Worker-->>Worker: Log warning, return false
end
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/config.ts (1)
305-309: Validate worker count configuration
Using a numeric conversion is fine, but consider edge cases (e.g., negative or extremely large values) that might degrade performance or cause resource exhaustion.test/end-to-end/indexing.test.ts (2)
866-869: Use a consistent logger
Consider usinglog.debugorlog.infoinstead ofconsole.logfor uniform logging and better filtering by log levels.
883-899: Minor spelling fix and comment clarity
• Line 883 has a small typo: "bundele" → "bundle".
• Hardcoding “79 data items” is fine for test clarity, but consider clarifying that this value may change if the bundle contents evolve.Apply the following diff for the spelling fix:
- // queue the bundele tx to populate the data root + // queue the bundle tx to populate the data root
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/config.ts(1 hunks)src/lib/data-root.ts(1 hunks)src/workers/data-verification.ts(1 hunks)test/end-to-end/indexing.test.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (4)
src/workers/data-verification.ts (1)
120-126: Ensure undefined dataRoot early check is appropriate
Returning immediately whendataRootis missing prevents wasted computation. The log message is clear about re-indexing needs, which is good. Ensure upstream logic properly detects and re-queues unindexed transactions for a seamless flow.src/lib/data-root.ts (1)
163-163: Confirm proper handling of string return type
Now thatcomputeDataRootreturns astring | undefined, verify all callers appropriately handle the string type (e.g., indexing, logging, comparisons).test/end-to-end/indexing.test.ts (2)
846-849: Extended test suite timeout
Allowing 120 seconds for background verification tests reduces flakiness in end-to-end environments. This appears practical.
878-878: Frequent verification interval
A 1-second interval helps accelerate tests, but may be too frequent for production. Consider a condition or environment check to differentiate test vs. production intervals.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #285 +/- ##
===========================================
- Coverage 71.85% 71.83% -0.03%
===========================================
Files 39 39
Lines 9804 9818 +14
Branches 563 564 +1
===========================================
+ Hits 7045 7053 +8
- Misses 2755 2761 +6
Partials 4 4 ☔ View full report in Codecov by Sentry. |
…am timeout These give us flexiblity when setting up integration tests. Not exposing them on docker-compose as data verification is not enabled by default.
dtfiedler
force-pushed
the
PE-7488-disable-e2e-tests
branch
from
cdf770d to
279394a
Compare
| @@ -65,7 +65,6 @@ describe('DataSources', () => { | |||
|
|
|||
| localStack = await new LocalstackContainer('localstack/localstack:3') | |||
| .withNetwork(network as any) | |||
| .withName('localstack') | |||
There was a problem hiding this comment.
this was causing warnings/errors when attempting to spin up local stack. this will give it a dynamic name for each execution avoiding conflicts
Error: (HTTP code 409) unexpected - Conflict. The container name "/localstack" is already in use by container "420421cdc2b4abb71994d9034928ab0fd352c77cc1e0cc8fe05eff9112297135". You have to remove (or rename) that container to be able to reuse that name. 
We need to index the bundle transaction before we can attempting to verify its data root.