Repository unit tests for ManifestLoader.get_full_manifest
#10147
Conversation
…cation There are a set of required mocks that `get_full_manifest` unit tests need. Instead of doing these mocks in each test, we've abstracted these mocks into a reusable function. I did try to do this as a fixture, but for some reaosn the mocks didn't actually propagate when I did that.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10147 +/- ##
==========================================
+ Coverage 88.58% 88.64% +0.05%
==========================================
Files 180 180
Lines 22422 22422
==========================================
+ Hits 19863 19876 +13
+ Misses 2559 2546 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
tests/unit/parser/test_manifest.py
Outdated
| return mock_project | ||
|
|
||
|
|
||
| @pytest.fixture |
There was a problem hiding this comment.
Shoude this live somewhere in the utils being used by other unittests?
There was a problem hiding this comment.
We can lift these 👍
| def set_required_mocks( | ||
| self, mocker: MockerFixture, manifest: Manifest, mock_adapter: MagicMock | ||
| ): | ||
| mocker.patch("dbt.parser.manifest.get_adapter").return_value = mock_adapter |
There was a problem hiding this comment.
Is it possible to do the patch inside the fixture? So users of those fixtures can just request the fixture then the patch is done?
There was a problem hiding this comment.
I tried doing so but, as noted in the commit message of 791ec1d, the mocks stopped propagating when I did so. I can take another stab at it, but can't make any promises. It definitely would be nice if we could do it as a fixture though.
There was a problem hiding this comment.
Converting it to a fixture does seem to work now. Not sure why it didn't before and why it does now 🤔 🤷
There was a problem hiding this comment.
I wonder whether it was not imported properly before
This wasn't working before, but it does now. Not sure why.
resolves #9865
Problem
ManifestLoader.get_full_manifest is one of the central functions of dbt-core. It is what is generally used for loading the manifest. However, previously we had no unit tests for it.
Solution
Add unit tests for
ManifestLoader.get_full_manifest. So in reality,get_full_manifestis more of an orchestrator method and thus offloads a fair bit of the work to other functions. We should separately ensure the sub functions get appropriately unit tested. With that said,get_full_manifesthas itself some logical branches that we want to guarantee with testing. The tests we've added test these logical branchesresetargumentwrite_perf_infoargumentPARTIAL_PARSE_FILE_DIFFflagChecklist