Skip to content

PoC: add context files blind test#4799

Closed
Michal-Leszczynski wants to merge 1 commit into
masterfrom
ml/add-ai-files-blind
Closed

PoC: add context files blind test#4799
Michal-Leszczynski wants to merge 1 commit into
masterfrom
ml/add-ai-files-blind

Conversation

@Michal-Leszczynski

Copy link
Copy Markdown
Collaborator

No description provided.

@Michal-Leszczynski

Copy link
Copy Markdown
Collaborator Author

@copilot review changes in this PR

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds default keyspace filtering to exclude temporary keyspaces and introduces an integration test to validate that tmp* keyspaces are not included in backup targets.

Changes:

  • Set default backup task keyspace patterns to include all keyspaces except those matching tmp*.
  • Add an integration test verifying GetTarget excludes tmp* keyspaces even when requested.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
pkg/service/backup/service_backup_integration_test.go Adds an integration test asserting tmp* keyspaces are excluded from backup targets.
pkg/service/backup/model.go Changes default task properties to filter out tmp* keyspaces by default.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}
}

func TestTmpKsIntegration(t *testing.T) {

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

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

The test name is ambiguous and uses an abbreviation (TmpKs) that doesn’t describe the behavior under test. Consider renaming to something behavior-focused like TestBackupGetTargetExcludesTmpKeyspaces (or similar), so failures are easier to interpret in CI output.

Suggested change
func TestTmpKsIntegration(t *testing.T) {
func TestBackupGetTargetExcludesTmpKeyspacesIntegration(t *testing.T) {

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ok

Transfers: scyllaclient.TransfersFromConfig,
Method: MethodRclone,
Continue: true,
Keyspace: []string{"*", "!tmp*"},

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

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

Changing the default keyspace selection to ["*", "!tmp*"] introduces a behavior change: tmp* keyspaces will be excluded by default system-wide, potentially even when a caller explicitly requests them (as the new integration test appears to verify). If this is intended policy, it should be explicitly documented and ideally configurable; if not intended, consider applying the exclusion only when the user does not provide a keyspace property (i.e., treat it as a default rather than an override).

Suggested change
Keyspace: []string{"*", "!tmp*"},
Keyspace: []string{"*"},

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

they won't be excluded when explicitly requested

Comment on lines +3110 to +3114
testBucket = "backuptest-tmpks"
testKeyspaceRegular = "ks"
testKeyspaceTmp = "tmpks"
)

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

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

Using a fixed bucket name (and very generic keyspace names) can cause collisions or cross-test interference when tests run concurrently or reuse shared infrastructure (e.g., shared MinIO/S3 or shared clusters). Prefer generating unique names per test run (e.g., include t.Name() and a random/UUID suffix) to improve isolation and reduce flakes.

Suggested change
testBucket = "backuptest-tmpks"
testKeyspaceRegular = "ks"
testKeyspaceTmp = "tmpks"
)
testKeyspaceRegular = "ks"
testKeyspaceTmp = "tmpks"
)
testBucket := fmt.Sprintf("backuptest-tmpks-%d", time.Now().UnixNano())

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ok nitpick

Copilot AI commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

@copilot review changes in this PR

Here's my review of the changes:

model.go — Default keyspace filter

Adding Keyspace: []string{"*", "!tmp*"} to defaultTaskProperties() is a breaking behavior change: all backup tasks that don't explicitly set a keyspace filter will now silently exclude keyspaces starting with tmp. Previously the default was no filter (all keyspaces included). Existing users with tmp* keyspaces would lose backup coverage without any warning.

Also note that the golden test everything.golden.json (used by TestGetTargetIntegration) was generated without this default filter, so this change likely breaks that test.

TestTmpKsIntegration — Multiple bugs

The core misunderstanding: json.Unmarshal replaces (not merges) the default Keyspace field. So when the test sets props["keyspace"] = ["ks", "tmpks"], the default exclusion !tmp* is no longer in effect.

Scenario 1 (lines 3128–3147): keyspace: ["ks", "tmpks"]

  • Override makes the filter ["ks", "tmpks"] → both keyspaces are included
  • Line 3142: asserts tmpks is NOT in units — wrong, it will be present
  • Line 3145: asserts 2 units — correct count, but contradicts the previous assertion

Scenario 2 (lines 3149–3164): keyspace: ["ks"]

  • Override makes the filter ["ks"] → only ks matches → 1 unit
  • Line 3162: asserts len == 2wrong, should be 1

Scenario 3 (lines 3166–3181): keyspace: ["tmpks"]

  • Override makes the filter ["tmpks"]tmpks is included
  • Line 3176: asserts tmpks is NOT in units — wrong
  • Line 3180: error message says "Expected 2 units" but checks != 1inconsistent

Missing coverage

None of the three scenarios test the actual new feature (default !tmp* exclusion). The important test case — calling GetTarget without specifying a keyspace, so the default ["*", "!tmp*"] kicks in — is never exercised. That would be the scenario that validates the model.go change actually works.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants