-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
VRF-897: refactoring VRF v2 and V2 Plus e2e tests #12208
VRF-897: refactoring VRF v2 and V2 Plus e2e tests #12208
Conversation
I see that you haven't updated any README files. Would it make sense to do so? |
) | ||
if err != nil { | ||
return nil, nil, fmt.Errorf("err: %w", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about killing most of the if-else nesting by reversing the logic:
// just for this case do this...
if *testConfig.VRFv2.General.UseExistingEnv && *testConfig.VRFv2.ExistingEnvConfig.ExistingEnvConfig. CreateFundSubsAndAddConsumers {
consumer, err := env.ContractLoader.LoadVRFv2LoadTestConsumer(*commonExistingEnvConfig.ConsumerAddress)
if err != nil {
return nil, nil, fmt.Errorf("err: %w", err)
}
consumers = append(consumers, consumer)
subIDs = append(subIDs, *testConfig.VRFv2.ExistingEnvConfig.SubID)
return subIDs, consumers, nil
}
// for all other cases, do this...
consumers, subIDs, err = SetupNewConsumersAndSubs(
env,
coordinator,
testConfig,
linkToken,
numberOfConsumerContractsToDeployAndAddToSub,
numberOfSubToCreate,
l,
)
if err != nil {
return nil, nil, fmt.Errorf("err: %w", err)
}
return subIDs, consumers, nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, just I usually prefer the most easiest solution
) | ||
if err != nil { | ||
return nil, nil, fmt.Errorf("err: %w", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember the comment where I suggested to reorganize the code by flipping the logic? This can be applied here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left some comments, but nothing that can't be addressed later or fixed/updated in the next PR.
Quality Gate passedIssues Measures |
* VRF-897: refactoring VRF v2 and V2 Plus e2e tests * VRF-897: fixing test * VRF-897: refactoring VRF v2 load test * VRF-897: fixing v2 load test * VRF-897: adding possibility to run vrf v2 test against existing env * VRF-897: removing code * VRF-897: fixing lint issue * VRF-897: finishing V2 test refactoring * VRF-897: fixing BHS test * VRF-897: fixing VRF V2 Basic test * VRF-897: fixing VRF V2 Basic test * VRF-897: small refactoring * VRF-897: fixing lint issues * VRF-897: fixing VRF v2 test * VRF-897: fixing VRF v2 test * VRF-897: fixing VRF v2 test * VRF-897: fixing VRF v2 test * VRF-897: fixing VRF v2 test * VRF-897: adding VRF Owner test * VRF-897: fixing VRF Owner test * VRF-897: adding BHS Load Test * VRF-897: rerun tests * VRF-897: removing perf test to run setting * VRF-897: removing logging * VRF-897: small fix * VRF-897: fixing compilation issues * VRF-897: fixing lint issues * VRF-897: fixing lint issues * VRF-897: refactoring * VRF-897: fixing lint * VRF-897: fixing vrfv2plus test * VRF-897: fixing vrfv2plus and vrfv2 test * VRF-897: fixing vrfv2plus and vrfv2 test * VRF-897: fixing vrfv2plus and vrfv2 test * VRF-897: fixing vrfv2plus and vrfv2 test * VRF-897: fixing vrfv2plus * VRF-897: fixing vrfv2plus test * VRF-897: fixing vrfv2plus test * VRF-897: PR comments * VRF-897: adding default value for bhs load test * VRF-897: adding default value for bhs load test * VRF-897: adjusting default value for bhs load test * VRF-897: fixing load test for existing env * VRF-897: fixing sub funds return to eoa wallet
PR covers multiple tasks:
bhs_test_duration
use_existing_env
is true, then it will not deploy new contracts, but will use specified vrf coordinator and keyhashSetupNewConsumersAndSubs
to the start of each test so that each test creates its own sub and consumer. Also, removed sub and consumer deployment from the main setup env script