Page MenuHomePhabricator

add postgres context variant to dagster-graphql
ClosedPublic

Authored by aj.nadel on Jun 30 2020, 11:51 PM.

Details

Test Plan

new tox.ini for additional tests that pull in dagster-postgres

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

aj.nadel created this revision.Jun 30 2020, 11:51 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 1 2020, 12:16 AM
Harbormaster failed remote builds in B14450: Diff 17741!
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 1 2020, 1:29 AM
Harbormaster failed remote builds in B14453: Diff 17745!
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 1 2020, 2:11 AM
Harbormaster failed remote builds in B14455: Diff 17747!
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 1 2020, 5:21 PM
Harbormaster failed remote builds in B14485: Diff 17780!
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 1 2020, 6:03 PM
Harbormaster failed remote builds in B14494: Diff 17789!
aj.nadel requested review of this revision.Jul 1 2020, 6:29 PM
aj.nadel updated this revision to Diff 17847.Jul 1 2020, 10:58 PM

move TestPostgresInstance into dagster.utils.test

sashank added a subscriber: sashank.Jul 2 2020, 4:11 PM

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?

.buildkite/pipeline.py
237

Delete comment

238

Why is this False?

python_modules/dagster-graphql/dagster_graphql_tests/graphql/graphql_context_test_suite.py
34

Delete comment

194

Delete comment

218

Hm I don’t see a try at line 240 - could you explain why we need it in a comment here?

386

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

aj.nadel marked 5 inline comments as done.Jul 2 2020, 6:53 PM
aj.nadel added inline comments.
.buildkite/pipeline.py
238

Ah no reason -- I copied these params from another ModuleBuildSpec, but weren't meant to be permanent.

python_modules/dagster-graphql/dagster_graphql_tests/graphql/graphql_context_test_suite.py
218

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.

aj.nadel updated this revision to Diff 17925.Jul 2 2020, 6:57 PM
aj.nadel marked 2 inline comments as done.

remove postgres_conditional_skip and move matrix tests to avoid excessive testing machinery

aj.nadel updated this revision to Diff 17955.Jul 2 2020, 8:58 PM

add coverage cmds to tox_postgres

aj.nadel updated this revision to Diff 17965.Jul 2 2020, 10:30 PM

hopefully last typo

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.

python_modules/dagster-graphql/dagster_graphql_tests/graphql/test_assets.py
73

Small comment here explaining what key we're sorting by would be helpful.

python_modules/dagster/dagster/utils/test/postgres_instance.py
70–73

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:

  1. 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:
    • python_modules/libraries/dagster-postgres/dagster_postgres_tests/conftest.py
    • examples/docs_snippets/docs_snippets_tests/intro_tutorial_tests/conftest.py
    • examples/airline_demo/airline_demo_tests/conftest.py
  2. 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.

sashank accepted this revision.Jul 6 2020, 9:35 PM
This revision is now accepted and ready to land.Jul 6 2020, 9:35 PM
aj.nadel updated this revision to Diff 18221.Jul 7 2020, 9:39 PM
aj.nadel marked 3 inline comments as done.

add explanation comment

aj.nadel retitled this revision from add postgress context variant to dagster-graphql to add postgres context variant to dagster-graphql.Jul 7 2020, 10:20 PM
This revision was automatically updated to reflect the committed changes.