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

chore: pytest path cleanup and port management #2734

Merged
merged 4 commits into from
Mar 5, 2024
Merged

Conversation

tychoish
Copy link
Contributor

@tychoish tychoish commented Mar 5, 2024

This contains two major changes:

  • we'll use either the release or the debug (in that order of
    preference) build if they exist. If neither exists, 🤷, that seems
    like a reasonable failure mode for integration tests. (Inspired by
    pytests fail locally if target/release is not available #2730)

  • randomly select an unused port, and use that (rather than the
    default postgres port.) this means we can run tests in parallel (and
    doing so halves the runtime on my laptop.) This had been on my list
    for a while, but I hadn't yet been bothered by the runtime.

Closes #2730

Copy link
Contributor

@talagluck talagluck left a comment

Choose a reason for hiding this comment

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

Just one suggestion in a couple of places. I'll wait to merge #2732.

Comment on lines 84 to 98
with tests.tools.env("GLAREDB_PORT", str(glaredb_connection.info.port)):
with tests.tools.env("DBT_USER", glaredb_connection.info.user):
res: dbtRunnerResult = dbtRunner().invoke(
[
"run",
"--project-dir",
dbt_project_path,
"--profiles-dir",
dbt_project_path,
"-m",
model_name,
]
)

assert res.success is True
Copy link
Contributor

Choose a reason for hiding this comment

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

The nested with block looks kind of gnarly. Maybe something like this instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

huh! amazing!

@@ -4,9 +4,9 @@ glaredb_dbt_test:
dbname: default
host: 127.0.0.1
pass: ""
port: 5432
port: "{{ env_var('GLAREDB_PORT') | as_number }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a welcome improvement.

@tychoish tychoish enabled auto-merge (squash) March 5, 2024 13:17
@tychoish tychoish merged commit 8bc6f47 into main Mar 5, 2024
25 checks passed
@tychoish tychoish deleted the tycho/pytest-cleanup branch March 5, 2024 13:38
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.

pytests fail locally if target/release is not available
2 participants