Skip to content

Add random name suffix to SQL Server test tables#28506

Merged
ebyhr merged 1 commit intotrinodb:masterfrom
LBPeraza:LBPeraza/sql-server-test-names
Mar 3, 2026
Merged

Add random name suffix to SQL Server test tables#28506
ebyhr merged 1 commit intotrinodb:masterfrom
LBPeraza:LBPeraza/sql-server-test-names

Conversation

@LBPeraza
Copy link
Member

@LBPeraza LBPeraza commented Mar 3, 2026

Description

Multiple test functions create tables with names from testTableNameTestData, which can cause flaky failures when running them concurrently. Adding these random suffixes should make sure the table names never collide.

Example stack trace:

io.trino.testing.QueryFailedException: line 1:1: Table 'sqlserver.dbo.lowercase' already exists
	at io.trino.testing.AbstractTestingTrinoClient.execute(AbstractTestingTrinoClient.java:138)
	at io.trino.testing.DistributedQueryRunner.executeInternal(DistributedQueryRunner.java:631)
	at io.trino.testing.DistributedQueryRunner.executeWithPlan(DistributedQueryRunner.java:613)
	at io.trino.testing.QueryAssertions.assertDistributedUpdate(QueryAssertions.java:112)
	at io.trino.testing.QueryAssertions.assertUpdate(QueryAssertions.java:66)
	at io.trino.testing.QueryAssertions.assertUpdate(QueryAssertions.java:60)
	at io.trino.testing.AbstractTestQueryFramework.assertUpdate(AbstractTestQueryFramework.java:419)
	at io.trino.testing.AbstractTestQueryFramework.assertUpdate(AbstractTestQueryFramework.java:414)
	at io.trino.plugin.sqlserver.TestSqlServerConnectorTest.testCreateAndDropTableWithSpecialCharacterName(TestSqlServerConnectorTest.java:184)

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Mar 3, 2026
@github-actions github-actions bot added the sqlserver SQLServer connector label Mar 3, 2026
@LBPeraza LBPeraza marked this pull request as ready for review March 3, 2026 00:47
@LBPeraza LBPeraza requested a review from ebyhr March 3, 2026 00:47
Copy link
Contributor

@chenjian2664 chenjian2664 left a comment

Choose a reason for hiding this comment

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

we seems not run the test concurrently? It's better to share the logs of the failure test

*/
private static String addNameSuffix(String name)
{
return name.replaceFirst("(\\s*)$", randomNameSuffix() + "$1");
Copy link
Contributor

Choose a reason for hiding this comment

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

It's hard to read the test about the table name, does the fix of testTableNameTestData works ?

Copy link
Member

@ebyhr ebyhr Mar 3, 2026

Choose a reason for hiding this comment

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

Another approach is to use AbstractTestQueryFramework#executeExclusively method. It still has a shared problem - a failure may affect other tests since the relevant test don't have finally block to drop tables.

Or just changing the prefix in each test likes #28508 might be sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe those randomized suffixes are good enough.
True, we can move drop to finally, and iirc synapse does not support "IF EXIST" in drop statement

Copy link
Contributor

@marcinsbd marcinsbd Mar 3, 2026

Choose a reason for hiding this comment

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

@LBPeraza is adding randomized suffix connected with running concurrently profiles|suites: weekly and nightly?

@ebyhr
Copy link
Member

ebyhr commented Mar 3, 2026

@chenjian2664 It runs concurrently since AbstractTestQueryFramework has @TestInstance(PER_CLASS) & @Execution(CONCURRENT).

@chenjian2664
Copy link
Contributor

@ebyhr Thanks for the explanation

@ebyhr
Copy link
Member

ebyhr commented Mar 3, 2026

There are 3 usages of testTableNameTestData, and each test has a different prefix:

  • testCreateAndDropTableWithSpecialCharacterName: (no prefix)
  • testRenameColumnNameAdditionalTests: tcn_
  • testRenameFromToTableWithSpecialCharacterName: test_rename_source_

I'm not sure where the conflict is occurring. TestSqlServerConnectorTest vs BaseConnectorTest?

@LBPeraza
Copy link
Member Author

LBPeraza commented Mar 3, 2026

we seems not run the test concurrently? It's better to share the logs of the failure test

I've added a failure stack trace to the PR description.

There 3 usages of testTableNameTestData, and each test has a different prefix:

  • testCreateAndDropTableWithSpecialCharacterName: (no prefix)
  • testRenameColumnNameAdditionalTests: tcn_
  • testRenameFromToTableWithSpecialCharacterName: test_rename_source_

I'm not sure where the conflict is occurring. TestSqlServerConnectorTest vs BaseConnectorTest?

testRenameFromToTableWithSpecialCharacterName initially creates the table with a test_rename_source_ prefix, but then renames it to the name without a prefix, which conflicts with the table created in testCreateAndDropTableWithSpecialCharacterName.

@LBPeraza LBPeraza force-pushed the LBPeraza/sql-server-test-names branch from 2e05751 to c46f2b8 Compare March 3, 2026 15:12
Multiple test functions create tables with names from
testTableNameTestData, which can cause flaky failures when running them
concurrently. Adding these random suffixes should make sure the table
names never collide.
@LBPeraza LBPeraza force-pushed the LBPeraza/sql-server-test-names branch from c46f2b8 to 6963ffd Compare March 3, 2026 15:17
@ebyhr ebyhr merged commit 234634d into trinodb:master Mar 3, 2026
23 checks passed
@github-actions github-actions bot added this to the 480 milestone Mar 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed sqlserver SQLServer connector

Development

Successfully merging this pull request may close these issues.

4 participants