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.
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
Refactor pytest unit tests to dbt unit tests #346
base: main
Are you sure you want to change the base?
Refactor pytest unit tests to dbt unit tests #346
Changes from 43 commits
9d8f0c8
298f373
906bec2
e994408
34bbab9
4b66d1f
79f7e27
cecf337
63a7d86
6e709db
9d53c9a
3425fdf
c3ba7f7
10456ef
a1f10df
5972788
20598fb
282eeee
c321197
8a1796e
c0aba5f
922ba07
3a4f677
c870130
7386371
76f2c7f
697bafd
616da99
653e1ae
50ff2e8
68f9f87
a3d9c1e
1dd415e
4ef2503
83bd23b
6f4335e
947868d
8c879f7
95b3a60
555671e
f198262
621e429
c82a36f
0ae02cc
99a50c8
cd35ef9
ac415db
7e44907
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidbooke4 I understand that these files aren't needed by a package user, but what is the benefit of removing them? They'll be added back the next time the person runs dbt-deps, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you're right - I forgot that would be the case! I'll update the comments I made in the README based on that fact.
One consequence of defining some of these project variables in
dbt_project.yml
within the package is that these variables will carry through to someone's dbt project if they've installed dbt-ga4 but haven't set new values for these variables in their owndbt_project.yml
. So models such asstg_ga4__page_conversions
andstg_ga4__derived_session_properties
will be enabled and have fields created based on the variables I've added todbt_project.yml
in the package.I'm trying to do some exploring to see if there's an alternative, but what are your thoughts on that? Do you think that'd be okay or do we need to find a way so these models aren't enabled if someone doesn't set those variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shoot, yea that's a problem because I don't think many package users use those advanced features or set those variables.
Can you look at alternatives?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found an alternative that boils down to creating more repo environment variables and updating the conditional enabled/disabled logic in some models to look for those environment variables.
However, now I'm back to an earlier problem where the project is unable to compile because unit tests are defined for models that are disabled. There's an open issue and PR related to this (with commits as recent as last week), so a fix should be in place soon 🤞.
What are your thoughts on waiting for this fix to be in place before merging this PR? The alternative would be to remove the dbt unit tests and add the
pytest
tests back in for the few models that are enabled based on setting the conversion events and derived user/session properties variables. I'd add in a TODO for replacing thosepytest
tests with unit tests if we decide to proceed with that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @adamribaudo-velir! I wanted to give the update that the PR I mentioned in my comment above was merged last week. That means we're probably at least a couple weeks out before it's included in a dbt version release, but I don't think there's any rush to get my PR merged so that should be fine. Let me know what you think though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Initially replied in the main thread)
Sounds great! Yes, let's wait until this is released in dbt-core. Thanks for staying on top of it.