Ensure unit test models can depend on external nodes #9343
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
QMalcolm
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9343 +/- ##
=======================================
Coverage 86.93% 86.94%
=======================================
Files 187 187
Lines 24954 24953 -1
=======================================
+ Hits 21695 21696 +1
+ Misses 3259 3257 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
QMalcolm
force-pushed
the
qmalcolm--8944-unit-test-models-dependent-on-external-nodes
branch
from
7fa5e5c
to
24114a3
Compare
… postgres max Was getting test failures due to resulting lengthy model names being created by unit test task in the functional test
…external node point to it Previously I was getting an error about the columns for the external model not being fetchable from the database via the macro `get_columns_in_relation`. By creating a seed for the tests, which creates a table in postgres, we can then tell the external model that it's database schema and identifier (the relation) is that table from the seed without make the seed an actual dependency of the external model in the dag.
External nodes generally don't have paths, but in unit tests we write out all models to sql files (as this allows us to test them). Thus external nodes need to have their paths set.
This is necessary because when executing a unit tests, the columns associated with a model in the database are retrieved. For this to be possible, the model must exist in the database, thus we must run the associated models at least once first.
QMalcolm
force-pushed
the
qmalcolm--8944-unit-test-models-dependent-on-external-nodes
branch
from
24114a3
to
b57a7f7
Compare
…an external node Previously we were only pseudo creating an external package for testing how unit tests work with external nodes. This was problematic because the package didn't actually exist and thus wasn't seen as accessible when running through dag dependencies. By actually creating the external package, we ensure that all the built in normal processes happen.
QMalcolm
force-pushed
the
qmalcolm--8944-unit-test-models-dependent-on-external-nodes
branch
from
d9774b8
to
adeb324
Compare
By flipping the logic from `not in` to `in` we can drop the exception and instead default to the model runtime config when the package isn't found. We're still trying to grok if there will be any fallout from this. The tests all pass, but that doesn't guarantee nothing bad will happen.
MichelleArk
reviewed
Jan 22, 2024
| raise PackageNotInDepsError(package_name, node=self._node) | ||
| yield dependencies[package_name] | ||
| if package_name in dependencies: | ||
| yield dependencies[package_name] |
There was a problem hiding this comment.
For posterity, thinking out loud about the implications of this change:
- This method is used to load the vars available for a particular jinja (model-scoped) context via the
varsmacro. - This method falls back on the running project's vars if the node has no associated packages. If it returns the wrong package -- this could lead to a 'no var found' issue at runtime.
- Before external models, it was not possible to have a model node exist in the project without a corresponding package, which is probably why this code path was not reachable
- With external nodes, this is totally possible now -- so explicitly supporting both paths feels totally reasonable, and correct!
MichelleArk
approved these changes
Jan 22, 2024
QMalcolm
deleted the
qmalcolm--8944-unit-test-models-dependent-on-external-nodes
branch
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #8944
Problem
We weren't sure if external nodes were already supported in unit test nodes. This PR added tests that identified that unit test nodes didn't have full support for external nodes, and then made the necessary changes to support them.
Solution
shadow manifesthave apathfor file writingpackages_for_nodeto gracefully allow for unknown dependenciesChecklist