CCIP-4303:Enabling in-memory test in integration-tests workflow #15388
Conversation
b-gopalswami
requested review from
winder,
rstout,
asoliman92,
makramkd,
dimkouv,
mateusz-sekara and
0xAustinWang
| @@ -18,8 +17,7 @@ import ( | |||
|
|
|||
| func TestInitialDeployOnLocal(t *testing.T) { | |||
| t.Parallel() | |||
| lggr := logger.TestLogger(t) | |||
| tenv, _, _ := testsetups.NewLocalDevEnvironmentWithDefaultPrice(t, lggr, nil) | |||
| tenv := changeset.NewMemoryEnvironmentWithJobsAndContracts(t, logger.TestLogger(t), 2, 4, nil) | |||
There was a problem hiding this comment.
not the best test to convert, may be try with fee boosting test. that has the longest running time. This one is just a basic test and should stay in docker. We already have a similar test in memory
There was a problem hiding this comment.
Also I see the runtime is not improved much( it's > 9min) in CI any idea why?
There was a problem hiding this comment.
This is just an example to prove it works. Instead of we convert some test to in-memory, we will eventually add in-memory tests like the one @asoliman92 wants to add for ccipreader.
The other test still runs for 350+ secs. This one previously ran for ~320 secs and now it is ~85 secs. So, clubbing in-memory and docker test might not be helpful. We should have in-memory tests as separate matrix.
…egration-tests workflow
.github/integration-tests.yml
Outdated
| test_cmd: cd integration-tests/smoke/ccip && go test ccip_messaging_test.go -timeout 12m -test.parallel=2 -count=1 -json | ||
| test_env_vars: | ||
| E2E_TEST_SELECTED_NETWORK: SIMULATED_1,SIMULATED_2 | ||
| E2E_JD_VERSION: 0.6.0 |
There was a problem hiding this comment.
Will remove it.
.github/integration-tests.yml
Outdated
| test_env_type: in-memory | ||
| runs_on: ubuntu-latest | ||
| triggers: | ||
| - PR Integration CCIP Tests |
There was a problem hiding this comment.
should be
- PR E2E Core Tests
- Nightly E2E Tests
There was a problem hiding this comment.
I don't think we need to call that as E2E Core. More over the triggers are adjusted accordingly. So, the above one is good for now. We had a discussion in our stand up to convert every test to sim backend in PR run and in nightly run, run the same test using docker setup. That is separate work needs to be done once we convert the existing test to sim backend.
| test_env_vars: | ||
| E2E_TEST_SELECTED_NETWORK: SIMULATED_1,SIMULATED_2 | ||
| E2E_JD_VERSION: 0.6.0 |
There was a problem hiding this comment.
Do these env vars make sense in the sim env?
| - '.github/workflows/integration-tests.yml' | ||
| - '.github/integration-tests.yml' |
There was a problem hiding this comment.
Are these pointing to the right files?
|
LGTM overall, just lemme know about the above comments, but the test changes seem fine. Once this is merged we can start cutting over other tests |
| - id: smoke/ccip/ccip_messaging_test.go:* | ||
| path: integration-tests/smoke/ccip/ccip_messaging_test.go |
There was a problem hiding this comment.
why only messaging_test? Can we do a regex to run all under ccip directory? or maybe even create a new directory ccip-1.6 that has only 1.6 related tests?
There was a problem hiding this comment.
This is just an example. I don't want to club the work to convert the test to sim backend in this PR. We can do that in a separate PR.
It's better to have it separate matrix to make it parallel and speed up the execution.
| repository: smartcontractkit/chainlink | ||
| ref: ${{ inputs.cl_ref }} | ||
|
|
||
| - name: 🧼 Clean up Environment |
|
@Tofel Thanks for your feedback. As I see all that feedbacks are improvements, will create a follow PR to polish this further but for now will proceed with this as this feature is much needed by team to make progress on other tests. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Adding postgres instance to integration_tests workflow and converted one test from docker to in-memory test.
If any new in-memory tests gets added, pass PG_INSTANCE test_env_vars as true.