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

Integration Tests Setup for MSSQL #57

Merged

Conversation

rahulreddy15
Copy link
Collaborator

@rahulreddy15 rahulreddy15 commented Jan 19, 2025

Integration tests for features added with Query Performance Monitoring.

Integrations tests cover these scenarios:

Perf Monitoring flag: enabled
The integration tests should run against the MSSQL Server 2017 & 2022
Verify if the stdout contains valid json's. Validate the stdout json's against defined json schemas

Perf Monitoring flag: enabled
The integration should be tested against a DB without the correct features / flags / extensions enabled

Perf Monitoring flag: disabled
The existing integration tests should run as expected without any errors

@rahulreddy15
Copy link
Collaborator Author

There is a know issue where Github Actions Runners cannot run older MSSQL versions.
For example MSSQL 2017.
Link to explanation here: actions/runner-images#10649 (comment)
Thread here: actions/runner-images#10649

So tests for the oldest supported versions are commented out.
They can be run on a x86 mac or on x86 debian for testing

Copy link

@sigilioso sigilioso left a comment

Choose a reason for hiding this comment

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

I've left some comments, ping me if you need any clarification.

tests/mssql_test.go Outdated Show resolved Hide resolved
tests/mssql_test.go Outdated Show resolved Hide resolved
tests/mssql_test.go Outdated Show resolved Hide resolved
tests/mssql_test.go Outdated Show resolved Hide resolved
tests/mssql_test.go Show resolved Hide resolved
tests/docker-compose.yml Show resolved Hide resolved
tests/simulation/sim_queries.go Outdated Show resolved Hide resolved
tests/testdata/mssql-schema.json Show resolved Hide resolved
tests/docker-compose.yml Outdated Show resolved Hide resolved
@abhinav1602
Copy link
Owner

There is a know issue where Github Actions Runners cannot run older MSSQL versions. For example MSSQL 2017. Link to explanation here: actions/runner-images#10649 (comment) Thread here: actions/runner-images#10649

So tests for the oldest supported versions are commented out. They can be run on a x86 mac or on x86 debian for testing

@rahulreddy15
I don't see anything specific to the MSSQL version, the issue seems to be related to the OS version only

@rahulreddy15
Copy link
Collaborator Author

rahulreddy15 commented Jan 21, 2025

There is a know issue where Github Actions Runners cannot run older MSSQL versions. For example MSSQL 2017. Link to explanation here: actions/runner-images#10649 (comment) Thread here: actions/runner-images#10649
So tests for the oldest supported versions are commented out. They can be run on a x86 mac or on x86 debian for testing

@rahulreddy15 I don't see anything specific to the MSSQL version, the issue seems to be related to the OS version only

Yeah. The ubuntu 16 base image that MSSQL Server 2017 is based on crashes in newer Ubuntu versions ex: 22, 24.
GHA use Ubuntu 22 and 24, so we cannot run the integration tests for MSSQL Server 2017.
Also refer to linked github thread that explains this issue better.

Copy link

@sigilioso sigilioso left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments! I've left another suggestion to simplify a bit further and another question. Let me know you thoughts 🙂

tests/mssql_test.go Outdated Show resolved Hide resolved
tests/mssql_test.go Outdated Show resolved Hide resolved
Comment on lines -34 to -38
func waitForMSSQLIsUpAndRunning(maxTries int) bool {
mssqlEnvVars := []string{
"ACCEPT_EULA=Y",
fmt.Sprintf("SA_PASSWORD=%s", dbPassword),
"MSSQL_PID=Developer",

Choose a reason for hiding this comment

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

Out of curiosity: the 'try to connect until the server is up' approach (instead of the new sleep 120 in the Makefile) doesn't work anymore? I'm asking because it might be faster if the server doesn't take that long to be up and running.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are doing more things than before:
-> Setup DB
-> Restore DB from backup
-> Enable QUERY STORE on restore DB so the new functionality can work.

Waiting for all this to be done seems more complicated. So defaulted to waiting 120 seconds so everything can be setup.

Copy link

@sigilioso sigilioso left a comment

Choose a reason for hiding this comment

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

LGTM

@rahulreddy15 rahulreddy15 merged commit 71705dd into epic_db_query_performance_monitoring_fork Jan 22, 2025
10 checks passed
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