Skip to content

[ZkTracer] feat: split AddressCollisionDelegationWarmingAndDeploymentTests per d…#2598

Open
amkCha wants to merge 5 commits intomainfrom
feat/split-address-collision-weekly
Open

[ZkTracer] feat: split AddressCollisionDelegationWarmingAndDeploymentTests per d…#2598
amkCha wants to merge 5 commits intomainfrom
feat/split-address-collision-weekly

Conversation

@amkCha
Copy link
Contributor

@amkCha amkCha commented Mar 16, 2026

…elegation type

This PR implements issue(s) #

Checklist

  • I wrote new tests for my new core changes.
  • I have successfully ran tests, style checker and build against my new changes locally.
  • If this change is deployed to any environment (including Devnet), E2E test coverage exists or is included in this
    PR.
  • I have informed the team of any breaking changes if there are any.

Note

Low Risk
Test-only refactor that changes how parameterized cases are generated/executed; primary risk is increased runtime or altered coverage if the new method split misconfigures parameters.

Overview
Refactors AddressCollisionDelegationWarmingAndDeploymentTests so delegation type is no longer part of the @MethodSource parameter set, and instead the suite is split into multiple @ParameterizedTest methods that each run the same input matrix with a fixed AccountDelegationType.

This keeps the underlying runWithParameters(...) logic the same while changing test enumeration/structure (and likely increasing the number of executed test cases via separate test methods per delegation type).

Written by Cursor Bugbot for commit 8fb92b7. This will update automatically on new commits. Configure here.

@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 9.70%. Comparing base (f23c478) to head (8fb92b7).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2598   +/-   ##
=====================================
  Coverage   9.70%   9.70%           
=====================================
  Files        454     453    -1     
  Lines      18530   18528    -2     
  Branches    2014    2014           
=====================================
  Hits        1799    1799           
+ Misses     16731   16729    -2     
Flag Coverage Δ *Carryforward flag
hardhat 96.63% <ø> (+0.10%) ⬆️ Carriedforward from 431d535
kotlin 0.10% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.
see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

}

return arguments.stream();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the input function is invoked for each of the tests below (you can check with debugging), while we could compute it only once, store it in the class and feed it to each test.

@letypequividelespoubelles letypequividelespoubelles added Arithmetization Arithmetization team is in charge or involved in this task Test Adding or enhancing test coverage labels Mar 17, 2026
Copy link
Contributor

@letypequividelespoubelles letypequividelespoubelles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You just splitted the test in 5 different tests to not have a single test timeout right ? I'm wondering if it's not better to just do a sampling of the test ...

@letypequividelespoubelles letypequividelespoubelles changed the title feat: split AddressCollisionDelegationWarmingAndDeploymentTests per d… [ZkTracer] feat: split AddressCollisionDelegationWarmingAndDeploymentTests per d… Mar 17, 2026
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

isDeployment == 1,
warming1,
warming2,
warming3));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant inputs() computation across six test methods

Low Severity

The inputs() static method is now used as @MethodSource by six separate test methods, causing it to be invoked six times — each time recomputing the identical combinatorial Stream<Arguments>. Previously, the delegation type was part of inputs(), so it was called once. As the reviewer @lorenzogentile404 noted, the result could be computed once and stored in a static field, then referenced by each test method.

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arithmetization Arithmetization team is in charge or involved in this task Test Adding or enhancing test coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants