new tox.ini for additional tests that pull in dagster-postgres
Nice work! It’s great to see the graphql tests finally run against Buildkite.
I feel like there might be a way to avoid some of the conditional skip logic we introduced here. It seems like the purpose of the activation_cond flag is to just not initialize the Postgres backed variants in the test_readonly_variants test. What do you think about running the tests for the matrix itself in an environment that contains dagster-postgres?
Also, sorry if I’m just forgetting a part of your explanation, but what is the purpose of the postgres_conditional_skip mark again? Will the commands from the original tox file somehow try to run these tests without the special mark?
Why is this False?
Hm I don’t see a try at line 240 - could you explain why we need it in a comment here?
There might be a better name for this, or at least we should have a long comment explaining it’s purpose, like we did for test_id in the comment block above.
Just some ideas (not necessarily better): should_run, is_active, mark_skipped=False
Ah no reason -- I copied these params from another ModuleBuildSpec, but weren't meant to be permanent.
Reference was to InstanceManagers.sqlite_instance_with_cli_api_run_launcher(), but we don't need it in a comment -- I was confused as to how yield in a try statement worked, but I understand now.
Woah this cleaned up nicely, great job! Glad to see the extra boolean flag and conditional mark go away.
Added one small question inline about the invariant checks. Otherwise, looks good to me. Will let @nate look over once and do the final sign off.
Small comment here explaining what key we're sorting by would be helpful.
Do we need these anymore, since this will only be initialized when we're in the right tox environment now?
Yeah, I definitely just overthought it. Two things:
- I think I should spend some more time de-duping the uses of postgres in tests (so I can take advantage of the container-launching setup). Just by grepping function names, it seems like there's a lot of repeated code:
- I left the checks in because I thought it might be a nice thing to raise on (in case test cases got changed/confused in the future and these get invoked without postgres). But if you don't think it adds any clarity over just letting package resolution, I'm totally fine to strip them out 👍
I think I should spend some more time de-duping the uses of postgres in tests
Feel free to do that in a follow up. What is here looks good. Also fair point on the invariant checks, makes sense to leave them in.