Better test structure #139
Conversation
This reverts commit 5abd4ec.
Codecov Report
@@ Coverage Diff @@
## master #139 +/- ##
===========================================
- Coverage 75.75% 29.53% -46.22%
===========================================
Files 8 8
Lines 1633 1534 -99
===========================================
- Hits 1237 453 -784
- Misses 396 1081 +685
Continue to review full report at Codecov.
|
|
More improvements made, at a good place to merge now |
| @@ -179,39 +172,80 @@ def test_generate_bq_schema_deprecated(): | |||
| gbq.generate_bq_schema(df) | |||
|
|
|||
|
|
|||
| class TestGBQConnectorIntegrationWithLocalUserAccountAuth(object): | |||
| @pytest.fixture(params=['local', 'service_path', 'service_creds']) | |||
There was a problem hiding this comment.
Woah. TIL about parameterized fixtures. Cool stuff.
|
|
||
| def setup_method(self, method): | ||
| @pytest.fixture(autouse=True) | ||
| def setup(self, project, credentials): |
There was a problem hiding this comment.
Since queries can be pretty long-running, I don't think we really want to use all 3+ credentials types here.
pandas_gbq/tests/test_gbq.py
Outdated
|
|
||
| def test_should_read_as_user_account(self): | ||
| _skip_local_auth_if_in_travis_env() | ||
| def test_should_read(project, credentials): |
There was a problem hiding this comment.
As opposed to my comment below, I do like this test using all three credentials types.
I think it's good to verify that with a speedy query like this one.
|
|
||
| def setup_method(self, method): | ||
| @pytest.fixture(autouse=True, scope='function') | ||
| def setup(self, project, credentials): |
There was a problem hiding this comment.
Ditto. I don't think we want all tests running with all credentials types, but we should have 1 test that does.
Generally only one of the sets of tests will run, because for the other auth types they'll be skipped. Indeed - this is no worse than the existing setup, where we had a bunch of copy & pasted code, but only one of the versions would run because the others would be skipped. Here we can also run tests using user auth, but that's the only growth in available tests. In a future version, we could pass explicit options from the command line on which auth type(s) should be used, rather than auto-skipping based on the environment. That would also have the advantage that the test suite won't pass if no authentication is available. But I don't think we need to couple those changes - this is strictly an improvement from the existing version - or is there something I'm missing? |
There was a problem hiding this comment.
I agree that this is definitely an improvement.
I'm not convinced that the tests will only run once in general, as my understanding of _get_private_key_contents() is that it will always read the file at _get_private_key_path(), so in that case both will run using service account keys.
If that's the case and we decide that the tests are taking too long, we can always go back an refactor.
|
@maxim-lian Did this pass on your Travis? |
|
I couldn't find a |
That's a good point. Should we skip one in Travis? Checking here, they do take about twice as long They do pass |
|
I've skipped one in Travis. Would be better to make the travis run command pass a command line which defined this to a parameterized test suite, but this is fine for the moment IMO |
Move towards more pytest idioms, reduce amount of code, reduce branching. This is a starter; still lots to do.
But can merge if ppl agree and keep making small improvments