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

test: Add integration-tests for Invoke-AzSqlDatabaseMigration #232

Merged

Conversation

fgheysels
Copy link
Member

Add addition integration tests for Invoke-AzSqlDatabaseMigration

#223

@netlify
Copy link

netlify bot commented Oct 19, 2021

👷 Deploy request for arcus-scripting pending review.
Visit the deploys page to approve it

🔨 Explore the source changes: 8d7b044

@fgheysels
Copy link
Member Author

Integration tests are failing because of a timeout while creating the connection to the Azure SQL database.

Only one of those tests that have a dependency on SQL fail, the other tests succeed. This seems to be due to the fact that 'auto-pause' is enabled on the Azure SQL database that we use to run our tests against.
I think that the amount of time that is required to resume the Azure SQL database exceeds the connection-timeout that is set by default when opening a connection to SQL.

We could workaround this by increasing the connection-timeout in the migrationscript, but maybe that's a bit overkill since it is only needed to make the tests succeed. Don't know if there are any other possibilities ? Maybe add some retry-functionality to the tests ? Or maybe add a 'warmup' section to the tests which is run only once right before our tests execute. In that warmup sequence we just try to open a connection to the DB. This should trigger the Azure SQL database to resume running.

@fgheysels
Copy link
Member Author

Integration tests are failing because of a timeout while creating the connection to the Azure SQL database.

Only one of those tests that have a dependency on SQL fail, the other tests succeed. This seems to be due to the fact that 'auto-pause' is enabled on the Azure SQL database that we use to run our tests against. I think that the amount of time that is required to resume the Azure SQL database exceeds the connection-timeout that is set by default when opening a connection to SQL.

We could workaround this by increasing the connection-timeout in the migrationscript, but maybe that's a bit overkill since it is only needed to make the tests succeed. Don't know if there are any other possibilities ? Maybe add some retry-functionality to the tests ? Or maybe add a 'warmup' section to the tests which is run only once right before our tests execute. In that warmup sequence we just try to open a connection to the DB. This should trigger the Azure SQL database to resume running.

I changed the 'BeforeEach' block to a 'BeforeAll' block, and added a SQL command there which would cause the SQL DB to wake up. Next to that, I also specified a ConnectionTimeout parameter. We'll see if this works out.

Copy link
Member

@stijnmoreels stijnmoreels left a comment

Choose a reason for hiding this comment

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

This is already a great addition for the integration tests on the SQL migration script. Very nice! You seem to have solved to problem with the sleeping database too, that's great!
Thanks for taking the time for these tests. It already covers the major pathways we would need to test.

@fgheysels
Copy link
Member Author

Tests are currently failing because a test that failed in the previous run, didn't properly clean up. This should be fixed now. can you try to re-trigger the pipeline @stijnmoreels ?

@tomkerkhove
Copy link
Contributor

/azp run CI - Arcus.Scripting

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@stijnmoreels stijnmoreels left a comment

Choose a reason for hiding this comment

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

Maybe indeed some description improvements, but otherwise, I think we're ready. Thanks for this! 💯

Oh, and my mention of the false positive remark.

@fgheysels
Copy link
Member Author

fgheysels commented Oct 29, 2021

Do we need something else before this PR can be merged ?
(I don't have the rights to merge PR's here; can only close it and enable auto-merge -which i've just done but those netlify tasks are still 'waiting' :P -).

@stijnmoreels @tomkerkhove @mbraekman

@fgheysels fgheysels enabled auto-merge (squash) October 29, 2021 09:58
@stijnmoreels stijnmoreels disabled auto-merge October 29, 2021 12:45
@stijnmoreels stijnmoreels merged commit 55884aa into arcus-azure:master Oct 29, 2021
@stijnmoreels
Copy link
Member

Thanks @fgheysels , for your hard work on this! 👍

@fgheysels fgheysels deleted the frgh/223_sqlintegrationtests branch October 29, 2021 12:51
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.

4 participants