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: unit tests with versioned refs #10889

Merged
merged 7 commits into from
Nov 14, 2024

Conversation

devmessias
Copy link
Contributor

Resolves #10880; Resolves #10528; Resolves #10623

Problem

The error raises due the method RefableLookup.find at dbt-my/core/dbt/contracts/graph/manifest.py:

Tracking the storage object, without specifying a version in the model we will obtain something like that for the Unit Test Node running phase

{'upstream_model_versioned': {}, 'upstream_model_versioned.v<LATEST>': {'demo_dbt_py': 'model.demo_dbt_py.upstream_model_versioned'}} 

Which it seems to me that is not right, because if I specified the latest_version for my model, I should get a Storage like this

{
    'upstream_model_versioned': {'demo_dbt_py': 'model.demo_dbt_py.upstream_model_versioned'}, 
    'upstream_model_versioned.v <LATEST> ': {'demo_dbt_py': 'model.demo_dbt_py.upstream_model_versioned'} 
} 

I run some different cases and obtain the following table

Status Model Ref Model Latest Test Ref Test Latest Expectation v on find Compilation Message
true true true false fail 1 depends on a node named 'upstream_model_versioned' with version '1' which was not found
true false false None fail 2 depends on a node named 'upstream_model_versioned' with version '2' which was not found
false None false false success None depends on a node named 'upstream_model_versioned' which was not found
false None true true success None depends on a node named 'upstream_model_versioned' which was not found
true true false None success 1 model.demo_dbt_py.upstream_model_versioned
true true true true success 1 model.demo_dbt_py.upstream_model_versioned
true false true false success 2 model.demo_dbt_py.upstream_model_versioned

However, even in cases where the tests were successful, the storage objects it seems that not properly constructed. For example, when using ref('upstream_model_versioned', v=1) I got this:

{'upstream_model_versioned': {}, 
 'upstream_model_versioned.v1': {'demo_dbt_py': 'model.demo_dbt_py.upstream_model_versioned'}}

This worked not because the storage structure was correct, but because there was a key named upstream_model_versioned.v1.

Solution

The only problem that I found is that the Node object for the UnitTest was not copying the value from latest_version prop if it was available

Checklist

  • I have read the contributing guide and understand what's expected of me.
  • I have run this code in development, and it appears to resolve the stated issue.
  • This PR includes tests, or tests are not required or relevant for this PR.
  • This PR has no interface changes (e.g., macros, CLI, logs, JSON artifacts, config files, adapter interface, etc.) or this PR has already received feedback and approval from Product or DX.
  • This PR includes type annotations for new and modified functions.

@devmessias devmessias requested a review from a team as a code owner October 21, 2024 12:27
@cla-bot cla-bot bot added the cla:yes label Oct 21, 2024
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

1 similar comment
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@github-actions github-actions bot added the community This PR is from a community member label Oct 21, 2024
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 62.78%. Comparing base (89caa33) to head (82b2056).
Report is 4 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (89caa33) and HEAD (82b2056). Click for more details.

HEAD has 41 uploads less than BASE
Flag BASE (89caa33) HEAD (82b2056)
unit 7 1
integration 35 0
Additional details and impacted files
@@             Coverage Diff             @@
##             main   #10889       +/-   ##
===========================================
- Coverage   89.10%   62.78%   -26.33%     
===========================================
  Files         183      183               
  Lines       23626    23630        +4     
===========================================
- Hits        21051    14835     -6216     
- Misses       2575     8795     +6220     
Flag Coverage Δ
integration ?
unit 62.78% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
Unit Tests 62.78% <0.00%> (-0.02%) ⬇️
Integration Tests 62.78% <0.00%> (-26.33%) ⬇️

@devmessias
Copy link
Contributor Author

Hello @dbeatty10. This is a PR with a very simple fix that closes 3 open issues. It's covered by functional tests, but I’m not sure why this class doesn’t have any unit tests, so I left that aside.

@graciegoheen graciegoheen added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Nov 4, 2024
@MichelleArk
Copy link
Contributor

Thank you for the contribution @devmessias!

This is looking good overall, but it looks like this PR is failing on unrelated functional tests with the same 'strategy' error for tests related to snapshots. We've been doing a lot of work on snapshots recently so it may just be an intermittent issue that has since been resolved. Could you please merge main into this branch and re-run the tests?

Otherwise, just a refactoring suggestion for clarity, as well as some examples of testing for expected errors in logs.

@devmessias
Copy link
Contributor Author

Hi @MichelleArk , the branch is now updated

@MichelleArk MichelleArk merged commit 1625eb0 into dbt-labs:main Nov 14, 2024
51 of 54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes community This PR is from a community member ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering
Projects
None yet
3 participants