Skip to content
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

Move more tests out of sql.spec.ts #13740

Merged
merged 13 commits into from
May 22, 2024
Merged

Move more tests out of sql.spec.ts #13740

merged 13 commits into from
May 22, 2024

Conversation

samwho
Copy link
Collaborator

@samwho samwho commented May 21, 2024

Description

This continues the work of #13734 in moving more and more tests out of sql.spec.ts and into various integration test files.

@samwho samwho added the firestorm Data/Infra/Revenue Team label May 21, 2024
@samwho samwho requested a review from mike12345567 May 21, 2024 16:40
@samwho samwho requested a review from a team as a code owner May 21, 2024 16:40
@samwho samwho changed the title Delete sql spec ts 2 Move more tests out of sql.spec.ts May 21, 2024
@samwho samwho enabled auto-merge May 22, 2024 09:35
@samwho samwho disabled auto-merge May 22, 2024 09:36
@@ -73,7 +74,7 @@ describe.each([
})

async function createTable(schema: TableSchema) {
table = await config.api.table.save(
return await config.api.table.save(
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️

@@ -1506,4 +1540,52 @@ describe.each([
expectQuery({ equal: { "1:1:name": "none" } }).toFindNothing())
})
})

// This will never work for Lucene.
// TODO(samwho): fix for SQS
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this meant to be done before merging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, @mike12345567 actually fixed this. Lemme "uncomment" the test. Good catch!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was just the comment that needed removing, the condition was already allowing the SQS test variant to run.

@samwho samwho enabled auto-merge May 22, 2024 13:03
@samwho samwho requested a review from adrinr May 22, 2024 13:04
Copy link
Collaborator

@mike12345567 mike12345567 left a comment

Choose a reason for hiding this comment

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

LGTM!

@samwho samwho merged commit 4a9c1ad into master May 22, 2024
10 checks passed
@samwho samwho deleted the delete-sql-spec-ts-2 branch May 22, 2024 16:07
@github-actions github-actions bot locked and limited conversation to collaborators May 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
firestorm Data/Infra/Revenue Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants