Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix default.vw_card_res_char tests to better match the reality of the view #110

Merged
merged 2 commits into from
Aug 31, 2023

Conversation

jeancochrane
Copy link
Contributor

@jeancochrane jeancochrane commented Aug 31, 2023

In Teams discussion with Nicole and Billy, I discovered two small problems with our default.vw_card_res_char tests:

  1. vw_card_res_char_unique_by_card_and_year is incorrect, since this table is actually unique by card, year, and PIN

[12:46 PM] Nicole Jardine (Assessor)
I believe this view is unique to pin, year, and card

  1. vw_card_res_char_renovation_fields_match is not actually useful, since it's just checking derived data, and the logic that generates these data is much more complicated than a simple test like "char_renovation should be the same value as char_recent_renovation"

[1:40 PM] Jean Cochrane (Assessor)

I'm wondering if the test checking whether char_renovation matches char_recent_renovation in default.vw_card_res_char needs some refinement... currently we have a naive check to make sure that these fields match, but I think in theory its plausible for them not to match, right? E.g. if a card was not renovated in a given year, but it was renovated in the prior two years, then we would have char_renovation=0 and char_recent_renovation=true; or vice versa, if a card was renovated in a given year, and it was also renovated in the prior three years, then we would have char_renovation=0 and char_recent_renovation=false. Do either of these seem like plausible scenarios? I don't totally get what char_renovation is indicating so I expect my assumptions may be wrong!

[1:41 PM] William Ridgeway (Assessor)
you're exactly right that they can have different values, perhaps it's not even worth implementing a test since all it would be doing is reiterating the logic that's already defined in the original view

This PR adjusts these two tests to better reflect these properties of the underlying data, and to fix tests that are currently failing (example).

We also make one quick tweak to the command that runs dbt tests in the case of a cache hit, which allows us to properly run tests against upstream models.

@jeancochrane jeancochrane changed the title Fix default.vw_card_res_char tests to better match the reality of the view Fix default.vw_card_res_char tests to better match the reality of the view Aug 31, 2023
@@ -62,7 +62,7 @@ jobs:
run: |
if [[ $MODIFIED_RESOURCES_ONLY == 'true' ]]; then
echo "Running tests on modified/new resources only"
dbt test -t "$TARGET" -s state:modified state:new --state "$STATE_DIR"
dbt test -t "$TARGET" -s state:modified state:new --defer --state "$STATE_DIR"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a small bug that I hadn't noticed before working on this PR. Prior to this change, the lack of a --defer flag meant that tests would fail when run against views or tables that were not built as part of the CI environment (see here for a sample failing workflow run). The lack of certain views or tables in a CI environment is intentional: We don't want to build models in CI unless their definition is new or has changed on the PR branch, which helps us speed up CI by only building and testing models that are being affected by the PR. The --defer tells dbt that if a model does not exist in the CI version of the DAG, but it does exist in the prod version of the DAG, then it should refer to the prod version for the purpose of running builds and tests. (The exception to this is models that have changed as part of the PR, which are always built in the CI environment due to the -s state:modified flag.)

Detailed docs on dbt's deferral system are here.

@jeancochrane jeancochrane marked this pull request as ready for review August 31, 2023 20:14
@jeancochrane jeancochrane requested a review from a team as a code owner August 31, 2023 20:14
@jeancochrane jeancochrane merged commit 3798bfa into master Aug 31, 2023
3 checks passed
@jeancochrane jeancochrane deleted the jeancochrane/fix-card-res-char-tests branch August 31, 2023 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants