Page MenuHomePhabricator

Pytest assertion rewrite on run storage tests
ClosedPublic

Authored by johann on Wed, Nov 18, 9:41 PM.

Details

Summary

add nice pytest assertions on run storage tests

This requires a pytest dep, so the test cases are moved from utils in dagster to dagster_tests.

Test Plan

integration

existing

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

johann edited the test plan for this revision. (Show Details)
Harbormaster returned this revision to the author for changes because remote builds failed.Wed, Nov 18, 10:13 PM
Harbormaster failed remote builds in B21367: Diff 25926!
Harbormaster failed remote builds in B21360: Diff 25919!
python_modules/dagster/dagster_tests/core_tests/storage_tests/utils/__init__.py
3–4

whats going on here? I dont get it

python_modules/dagster/dagster_tests/core_tests/storage_tests/utils/__init__.py
3–4

https://docs.pytest.org/en/stable/writing_plugins.html#assertion-rewriting

These test cases are imported into the various test files for each run storage. By default pytest rewrites asserts within test files to aid in debugging. E.g. assert len(foo) == 2 would normally just fail with an AssertionError, but with pytest you get detailed info like 1 == 2. Pytest doesn't override the asserts in imported libraries, unless called explicitly as above

python_modules/dagster/dagster_tests/core_tests/storage_tests/utils/__init__.py
3–4

You also need to call rewrite before the file is included, hence the isort skip

Whoops, the path was wrong. Verified the behavior this time.

a = 'hello'
assert a == 'no'

Before:

>       assert a == "no"
E       AssertionError

Now:

>       assert a == "no"
E       AssertionError: assert 'hello' == 'no'
E         - no
E         + hello
dgibson added inline comments.
python_modules/dagster/dagster_tests/core_tests/storage_tests/utils/__init__.py
3–4

ah nice - maybe worth a comment, I definitely was not aware of this either. very cool though

I wonder if the graphql test suite needs something similar? Maybe not since that just supplies fixtures i think

This revision is now accepted and ready to land.Fri, Nov 20, 4:53 PM
This revision was landed with ongoing or failed builds.Mon, Nov 23, 8:18 PM
This revision was automatically updated to reflect the committed changes.