test: move withdrawal tests to sim tests - #6507
Merged
Merged
Conversation
nazarhussain
commented
Mar 5, 2024
| if (!payloadId) throw Error("InvalidPayloadId"); | ||
|
|
||
| // 2. Get the payload | ||
| const payloadWithValue = await executionEngine.getPayload(ForkName.capella, payloadId); |
Contributor
Author
There was a problem hiding this comment.
I believe it's testing the engine API from the execution engine. Which should not be in our test suite.
The behavior passively is tested in sim tests anyway.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6507 +/- ##
============================================
- Coverage 61.72% 61.46% -0.27%
============================================
Files 555 556 +1
Lines 58204 58850 +646
Branches 1844 1850 +6
============================================
+ Hits 35927 36171 +244
- Misses 22238 22638 +400
- Partials 39 41 +2 |
Contributor
Performance Report鉁旓笍 no performance regression detected 馃殌馃殌 Significant benchmark improvement detected
Full benchmark results
|
jeluard
previously approved these changes
Mar 19, 2024
jeluard
left a comment
jeluard
left a comment
Contributor
There was a problem hiding this comment.
Not an expert in sim tests, but overall the approach sounds sensible to me. And always 馃憤 removing complexity :)
| */ | ||
| const result = [`Slot,${nodes.map((n) => n.beacon.id).join(", ,")}`]; | ||
| result.push(`,${nodes.map((_) => "Withdrawal Amount,Withdrawal Count").join(",")}`); | ||
| for (let s = 1; s <= slot; s++) { |
Contributor
Author
There was a problem hiding this comment.
Then TS would not allow to use s++, you can't mutate a const declared primitive.
| let row = `${s}`; | ||
| for (const node of nodes) { | ||
| const {withdrawalAmount, withdrawalCount} = store[node.beacon.id][s] ?? {}; | ||
| row += `,${withdrawalAmount ?? "-"},${withdrawalCount ?? "-"}`; |
Contributor
There was a problem hiding this comment.
Maybe some combination of Array#join and Array#map could be used here. Just for reference, for this PR looks ok.
Contributor
Author
There was a problem hiding this comment.
This iteration approach looks more readable.
| const validators: WithdrawalsData["validators"] = {}; | ||
|
|
||
| for (const withdrawal of withdrawals) { | ||
| withdrawalCount++; |
Contributor
There was a problem hiding this comment.
Looks like it can only ever end up being withdrawals.length ?
Contributor
Author
There was a problem hiding this comment.
Nice catch. was using different approach earlier so this variable left as counter.
| return { | ||
| id: `withdrawals_${nodeId}`, | ||
| match({forkConfig, epoch, node}) { | ||
| if (nodeId === node.id && epoch === forkConfig.CAPELLA_FORK_EPOCH) |
Contributor
There was a problem hiding this comment.
I'm surprised this line pass lints (no curly brasses).
Contributor
Author
There was a problem hiding this comment.
It was actually written as single line, but prettier changed it to this format. And our ESlint don't complaint on Prettier formatting.
| @@ -0,0 +1,98 @@ | |||
| /* eslint-disable @typescript-eslint/naming-convention */ | |||
Contributor
There was a problem hiding this comment.
According to lints, this directive is useless.
twoeths
reviewed
Mar 20, 2024
| ENGINE_PORT: 8551 | ||
| ETH_PORT: 8661 | ||
|
|
||
| - name: Pull geth withdrawals |
Member
There was a problem hiding this comment.
looks like we need to remove the whole file?
Contributor
Author
There was a problem hiding this comment.
Not yet, one other PR in progress then we can remove this file and a lot of utility code along it.
twoeths
reviewed
Mar 20, 2024
twoeths
reviewed
Mar 20, 2024
| let withdrawalAmount = BigInt(0); | ||
| const validators: WithdrawalsData["validators"] = {}; | ||
|
|
||
| for (const withdrawal of withdrawals) { |
Member
There was a problem hiding this comment.
what if it's empty withdrawals? is it scanned through assert() function below?
Contributor
Author
There was a problem hiding this comment.
Yes the assertion works on this logic.
if (nodeId === node.id && epoch === forkConfig.CAPELLA_FORK_EPOCH)
return AssertionMatch.Capture | AssertionMatch.Assert;So even if the above line withdrawals are empty, the assertion will still trigger and results in empty errors array.
twoeths
previously approved these changes
Mar 20, 2024
twoeths
left a comment
twoeths
left a comment
Member
There was a problem hiding this comment.
looks good to me, spent some time to see how capture() and assert() works. Nice work in SimulationTracker @nazarhussain . It's difficult for me to debug it through.
Contributor
Author
Both works based on the |
jeluard
approved these changes
Mar 20, 2024
Member
|
馃帀 This PR is included in v1.18.0 馃帀 |
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
4 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.
Motivation
Remove redundant tests
Description
Steps to test or reproduce