chore: update the performance tests to use @chainsafe/benchmark #7373
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #7373 +/- ##
============================================
+ Coverage 48.62% 50.39% +1.77%
============================================
Files 603 603
Lines 40516 40510 -6
Branches 2071 2208 +137
============================================
+ Hits 19700 20417 +717
+ Misses 20778 20053 -725
- Partials 38 40 +2 |
nazarhussain
force-pushed
the
nh/benchmark-update
branch
from
c66f2f3 to
bcdad5f
Compare
|
|
||
| // TODO: Diagnose why this benchmark failing after upgrade | ||
| // https://github.com/ChainSafe/lodestar/issues/7380 | ||
| bench.skip({ |
There was a problem hiding this comment.
Only this benchmark is failing, need to troubleshoot in particular later on.
| @@ -38,7 +38,7 @@ describe("validate gossip attestation", () => { | |||
| state, | |||
| bitIndex: i, | |||
| }); | |||
| expect(subnet).to.be.equal(subnet0); | |||
| assert.deepEqual(subnet, subnet0); | |||
There was a problem hiding this comment.
It seems weird that we need to use assert here...
| it("ArrayBuffer use after structuredClone transfer", () => { | ||
| const data = new Uint8Array(32); | ||
| data[0] = 1; | ||
| expect(data[0]).equals(1); | ||
| structuredClone(data, {transfer: [data.buffer]}); | ||
| // After structuredClone() data is mutated in place to hold an empty ArrayBuffer | ||
| expect(data[0]).equals(undefined); | ||
| expect(data).deep.equals(new Uint8Array()); | ||
| }); |
There was a problem hiding this comment.
Did this get moved to a unit test or something?
There was a problem hiding this comment.
Looked closely and my observation was that it's not that useful test case. So removed it.
| it("Check is correct", () => { | ||
| for (const i of [0, 1, Math.floor(n / 2)]) { | ||
| expect(array[i]).to.equal(i, `Wrong value array[${i}]`); | ||
| expect(arrayWithProxy[i]).to.equal(i, `Wrong value arrayWithProxy[${i}]`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Did this get moved to a unit test?
There was a problem hiding this comment.
Same as above.
This reverts commit 8a0e51d.
|
It would be good to figure out #7380 before merging this |
|
there is no benchmark report / comment on this PR, this is no longer supported or just broken? |
Already looking into it. |
Motivation
Use the
@chainsafe/benchmarkfork for our performance tests. This will enable to run these tests on multiple JS runtimes.Description
Steps to test or reproduce