Skip to content

Ensure test threads termination even when test overridden#16991

Merged
findepi merged 2 commits intotrinodb:masterfrom
findepi:findepi/fix-sqlserver-testReadMetadataWithRelationsConcurrentModifications
Apr 13, 2023
Merged

Ensure test threads termination even when test overridden#16991
findepi merged 2 commits intotrinodb:masterfrom
findepi:findepi/fix-sqlserver-testReadMetadataWithRelationsConcurrentModifications

Conversation

@findepi
Copy link
Copy Markdown
Member

@findepi findepi commented Apr 12, 2023

For example BaseSqlServerConnectorTest overrides
testReadMetadataWithRelationsConcurrentModifications. Before the
change, the test invocation could leave background task running,
eventually leading to failure during AbstractTestQueryFramework.close
(in checkQueryInfosFinal()).

Fixes #15430

@findepi findepi added test no-release-notes This pull request does not require release notes entry labels Apr 12, 2023
@cla-bot cla-bot bot added the cla-signed label Apr 12, 2023
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The commit message is Ensure test threads termination even when test overridden - feels like a general change; but it is for specific test only.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

wait - did you miss throw failure here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

wait - did you miss throw failure here?

good catch!

The commit message is Ensure test threads termination even when test overridden - feels like a general change; but it is for specific test only.

yes, but i couldn't put testReadMetadataWithRelationsConcurrentModifications in the title line, could i?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually the following text was misleading to me. It was stating testReadMetadataWithRelationsConcurrentModifications as an "example" - which I interpreted as the change is more generic, and you are explaining why it is helpful using this specifi testcase.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

now that i have read the commit message, i see your point. reworded

@findepi findepi force-pushed the findepi/fix-sqlserver-testReadMetadataWithRelationsConcurrentModifications branch from b8680ee to 56af54b Compare April 12, 2023 13:52
@github-actions github-actions bot added hive Hive connector iceberg Iceberg connector tests:hive labels Apr 12, 2023
…rrentModifications

`BaseSqlServerConnectorTest` overrides
`testReadMetadataWithRelationsConcurrentModifications`. Before the
change, the test invocation could leave background task running,
eventually leading to failure during `AbstractTestQueryFramework.close`
(in `checkQueryInfosFinal()`).
@findepi findepi force-pushed the findepi/fix-sqlserver-testReadMetadataWithRelationsConcurrentModifications branch from 56af54b to f0ba4e4 Compare April 12, 2023 18:49
@findepi findepi merged commit 488c597 into trinodb:master Apr 13, 2023
@findepi findepi deleted the findepi/fix-sqlserver-testReadMetadataWithRelationsConcurrentModifications branch April 13, 2023 12:05
@github-actions github-actions bot added this to the 414 milestone Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed hive Hive connector iceberg Iceberg connector no-release-notes This pull request does not require release notes entry test

Development

Successfully merging this pull request may close these issues.

Flaky TestSqlServerConnectorTest.checkQueryInfosFinal test check: query is expected to be in a done state

3 participants