257 replace mocks #291
257 replace mocks #291
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #291 +/- ##
=======================================
Coverage 98.49% 98.49%
=======================================
Files 11 11
Lines 996 998 +2
=======================================
+ Hits 981 983 +2
Misses 15 15 ☔ View full report in Codecov by Sentry. |
tests/test_manager.py
Outdated
| assert ( | ||
| "Expected iterable metadata columns: ['model', 'realm', 'frequency', 'variable']" | ||
| in str(excinfo.value.__cause__) | ||
| ) |
There was a problem hiding this comment.
This is currently throwing me an error, I think the model column is missing.
There was a problem hiding this comment.
Weird - passes on my machine & CI.
Can you show me the error message you're getting?
There was a problem hiding this comment.
From pytest --cov-report=term-missing --cov=access_nri_intake, I get:
FAILED tests/test_manager.py::test_CatalogManager_load_non_iterable - assert "Expected iterable metadata columns: ['model', 'realm', 'frequency', 'variable']" in "Cannot add entry with iterable metadata columns: ['realm', 'frequency', 'variable'] to dataframe catalog with iterable metadata columns: ['model', 'realm', 'frequency', 'variable']. Please ensure that metadata ...
There was a problem hiding this comment.
@marc-white Is it still this same error that you're getting?
…cstr Co-authored-by: Marc White <[email protected]>
for more information, see https://pre-commit.ci
|
Extremely confused as to whats going on with test failures here - have moved back to draft |
Maybe you've got a mixed checkout locally with some out-of-date test data? |
|
The test conversions themselves look fine to me. |
|
I still get a test failure on a local checkout copy: |
|
Weird - all passing on my machine & CI - will double check whats up. |
Okay, I'm confident I know the source of this - can you confirm the version of This test is checking for the upstream error message. On reflection, this level of coupling is stupid, so I've replaced it with a try & a warning if it fails. Let me know if that solves the issue for you. |
|
Cool - I'm on 0.3 & I assume CI is too, so I guess that's the likely source of the error. Anyway, good catch. |
Closes #257, and makes some progress on #271.
Unfortunately I got mixed up and have removed a lot of the wrong mocks, but this PR currently replaces a lot of the (I think unnecessary) mocks in the
tests/test_cli.pyfile, as well as replacing a lot of calls to theosmodule withpathlib.Marked as draft because I still need to address the mocks in #257, but it seemed logical to bundle these all together.
@marc-white If you still remember the context of these tests, can you tell me if I'm violating any of the intentions with my changes?