Integration in memory tests overhaul - #23288
Merged
Merged
Conversation
Contributor
|
✅ No conflicts with other open PRs targeting |
kalverra
marked this pull request as ready for review
July 31, 2026 16:34
product-security-plaid-production
Bot
requested review from
Tofel,
george-dorin and
tvc-robsondebraga
July 31, 2026 16:34
jmank88
reviewed
Aug 4, 2026
jmank88
previously approved these changes
Aug 4, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (9)
core/store/models/common_test.go:80
- t.Parallel() inside the subtest uses the range variable
testfrom the outer loop without capturing it. Because subtests can start running after the loop continues, this can cause the wrong test case data to be used and lead to flaky tests.
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
t.Parallel()
core/store/models/common_test.go:141
- Same loop-variable capture issue with t.Parallel(): rebind
test := testinside the loop before starting parallel subtests to avoid races/flakiness.
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
t.Parallel()
core/store/models/common_test.go:226
- Same loop-variable capture issue with t.Parallel() in subtests: rebind the loop variable before starting the parallel subtest to avoid non-deterministic behavior.
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
t.Parallel()
core/store/models/common_test.go:117
- Same loop-variable capture issue with t.Parallel(): the subtest closure reads
testfrom the surrounding range loop without rebinding it, which can make the test non-deterministic under parallel execution.
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
t.Parallel()
core/store/models/common_test.go:206
- Same loop-variable capture issue with t.Parallel() in subtests: the closure uses
testfrom the outer range loop without rebinding it, which can lead to flaky results.
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
t.Parallel()
core/store/database_ddl.go:33
- Removing the gosec suppression here is likely to reintroduce a gosec warning for SQL string formatting (even though the identifier is validated/escaped). This may break CI linting; keep a targeted //nolint:gosec with justification.
// PostgreSQL does not support bound parameters for database identifiers.
_, err = db.ExecContext(ctx, fmt.Sprintf(dropDatabaseSQLFmt, quoted))
core/store/database_ddl.go:44
- Same as above: fmt.Sprintf-based SQL construction tends to trigger gosec; without a justified //nolint:gosec this can fail linting even though quotePostgresDBName validates/escapes the identifier.
// PostgreSQL does not support bound parameters for database identifiers.
_, err = db.ExecContext(ctx, fmt.Sprintf(createDatabaseSQLFmt, quoted))
.github/workflows/integration-in-memory-tests.yml:24
- The workflow_call input is named/described as a Slack channel ID, but the default fallback uses a channel name (#ccip-testing) and callers may pass #channel as well. Consider clarifying in the description that this accepts either a channel ID or a #channel name, to avoid confusing/misleading callers.
slack_channel_id:
description: "Slack channel ID to notify test results"
core/store/store_test.go:15
- Minor typo in comment: "without depending on on-disk paths" has a duplicated "on".
// insertFixtures must execute the given fixture SQL without depending on
// on-disk paths derived from runtime.Caller, so that pre-compiled binaries
…integrationInMemoryTestsOverhaul
jmank88
previously approved these changes
Aug 4, 2026
…integrationInMemoryTestsOverhaul
mchain0
previously approved these changes
Aug 5, 2026
Tofel
reviewed
Aug 5, 2026
Tofel
previously approved these changes
Aug 5, 2026
kalverra
enabled auto-merge
August 5, 2026 13:26
|
Tofel
approved these changes
Aug 5, 2026
jmank88
approved these changes
Aug 5, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.




Big Changes
core/store/to use embedded go string instead of looking for a file. This enables some CI optimizations, and makes sense to me, but could change behavior/intent in ways I'm not sure of. Calling in @jmank88 on this.Integration In-Memory Tests Improvements
These would run for a long time, were very complex, and burned a lot of money. They've been greatly simplified, made faster, and cheaper.
Comparison
Small Changes
core/store/