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

Fix: missing schema param in inferred id run test #478

Conversation

marcomoscasgr
Copy link
Contributor

@marcomoscasgr marcomoscasgr commented Nov 21, 2024

During a log_publisher test, the connection to the tracking DB happens when the npg_tracking schema file is available on disk under the proper directory. If not present on disk the test passes skipping the connection. To avoid unexpected connections from this particular test when the file exists,npg_tracking_schema property should be set to undef.

@marcomoscasgr marcomoscasgr self-assigned this Nov 21, 2024
Copy link
Member

@kjsanger kjsanger left a comment

Choose a reason for hiding this comment

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

Please add a comment in the test code alongside this so that it is clear why setting an undef argument has side-effects on a database (!!) Otherwise, someone may just remove it later.

@mgcam
Copy link
Member

mgcam commented Nov 22, 2024

Please add a comment in the test code alongside this so that it is clear why setting an undef argument has side-effects on a database (!!) Otherwise, someone may just remove it later.

Good point. This test relies on the ability of the parent class to infer id_run. One might say that this test can be dropped since it does not test any code in the loader itself. id_run is retrieved either from a database run record or from the {r,R}unParameters.xml file in this order. Since the test did not provide a database handle, it tried to retrieve db credentials from $HOME/.npg and connect to whatever database is defined there. If that fails, it falls back to the {r,R}unParameters.xml route. In CI no test database was provided, so the fallback was successfully executed. If the user has credentials for the prod database in $HOME/.npg, a connection to the database is made. The fact that the tests connects to a database came to light when the database in question was a test database (listed as live), which refused connection, resulting in test failure with a fatal error. Setting the database handle to undef signals to the code that it should not try the database route.

@kjsanger kjsanger merged commit 0912308 into wtsi-npg:devel Nov 26, 2024
4 of 6 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