Ccip-4333 Handle all test setups with unified interface for memory and docker tests #15581
Conversation
AnieeG
requested review from
winder,
rstout,
asoliman92,
makramkd,
dimkouv,
mateusz-sekara and
0xAustinWang
…CCIP-4333-interface-test
Flakeguard Summary
Found Flaky Tests ❌
|
Flakeguard Summary
Found Flaky Tests ❌
|
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
| @@ -805,35 +566,6 @@ func ConfirmRequestOnSourceAndDest(t *testing.T, env deployment.Environment, sta | |||
| return nil | |||
| } | |||
|
|
|||
| // TODO: Remove this to replace with ApplyChangeset | |||
| tenv, _ := testsetups.NewIntegrationEnvironment( | ||
| t, | ||
| changeset.WithMultiCall3(), | ||
| changeset.WithEnvironmentType(changeset.Memory), |
There was a problem hiding this comment.
I wonder whether that should be driven by the code or some sort of env variable. I think all of the tests using testsetups.NewIntegrationEnvironment should be executable with both inmem and docker env. That way, you don't need to recompile everything to switch the env. I can imagine sth like this (don't focus on the names or the impl details, it's to show the high level ideas)
func Test_Sth(t *testing.T) {
tenv, _ := testsetups.NewIntegrationEnvironment(
t,
changeset.WithMultiCall3(),
changeset.WithEnvironmentType(changeset.EnvDriven) // probably needs a better name
}
func NewIntegrationEnvironment(t *testing.T, opts ...changeset.TestOps) {
// ...
if testCfg.Type == changeset.EnvDriven {
envType := os.Getenv("CCIP_INT_TESTS_ENV")
if envType == "docker"{
return docker
} else { // default to inmem
return inmem
}
}There was a problem hiding this comment.
That enables a couple of nice things:
- fast tests on the CI checks, but we can run everything in docker upon merge to main (it requires only a different ENV variable in CI configuration)
- doesn't require re-compilation if you want to debug locally using inmem instead of the docker hardcoded
I can imagine the CI setup in which in-memory tests run always (every PR, merge queue, etc), for both repositories: chainlink, chainlink-ccip. However, additionally we can have some sanity nightly runs using docker being reported to our channel (or maybe after merge to main)
There was a problem hiding this comment.
What's stops us from doing something like this in the tests:
func Test_Something(t *testing.T) {
tenv, _ := testsetups.NewIntegrationEnvironment(
t,
changeset.WithMultiCall3(),
changeset.WithEnvironmentType(testhelpers.MustGetEnvTypeOrDefault(t)),
)
// .. rest of test code
}
// in package testhelpers
func MustGetEnvTypeOrDefault(t *testing.T) EnvType {
envType := os.Getenv("CCIP_V16_TEST_ENV")
if envType == "" || envType == "in-memory" {
return EnvTypeMemory
} else if envType == "docker" {
return EnvTypeDocker
}
t.Fatalf("env var CCIP_V16_TEST_ENV must be either 'in-memory' or 'docker', defaults to 'in-memory' if unset, got: %s", envType)
}
| @@ -59,6 +60,16 @@ type CCIPOCRParams struct { | |||
| ExecuteOffChainConfig pluginconfig.ExecuteOffchainConfig | |||
| } | |||
|
|
|||
| // Override overrides non-empty dst CCIPOCRParams attributes with non-empty src CCIPOCRParams attributes values | |||
There was a problem hiding this comment.
Idea is we will use DefaultOCRParams and only mention the non-nil param values to override from chainlink-deployments
| @@ -59,6 +60,16 @@ type CCIPOCRParams struct { | |||
| ExecuteOffChainConfig pluginconfig.ExecuteOffchainConfig | |||
| } | |||
|
|
|||
| // Override overrides non-empty dst CCIPOCRParams attributes with non-empty src CCIPOCRParams attributes values | |||
| // and returns the updated CCIPOCRParams. | |||
| func (c CCIPOCRParams) Override(overrides CCIPOCRParams) (CCIPOCRParams, error) { | |||
There was a problem hiding this comment.
Not particularly against this approach but we already have a paradigm (if we want to call it that) of overriding via a func(c CCIPOCRParams) CCIPOCRParams that noufel added, I think. Preferably we don't have two ways to do the same thing.
There was a problem hiding this comment.
Think I prefer the approach we have now due to no new dependency and less magic / reflection, which it seems like this mergo package would be doing.
There was a problem hiding this comment.
I did not know we already have one. Should I completely remove this?
There was a problem hiding this comment.
oh you mean the one in test config. ah I see okay cool I can remove this and make it similar to what used in test config
This introduces a common interface for
TestEnvironmentand standardizesNewEnvironmentcreation by takingTestEnvironmentas parameter.Reduces the need of creating separate set up code for memory and docker environment from integration tests. Both kind of environments can now be constructed from
NewIntegrationEnvironmentby passing applicableTestOps-WithEnvironmentType(memory/docker)I followed this -
If called from chainlink/deployments - Use
NewMemoryEnvironmentwithTestOpsto create test environments.If called from chainlink/integration-tests - Use
NewIntegrationEnvironmentwithTestOps. NewIntegrationEnvironment is capable of creating memory and docker both based on providedEnvironmentType