-
Notifications
You must be signed in to change notification settings - Fork 135
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?
Conversation
For stg_ga4__page_conversions
…. Not working yet
…the default channel grouping macro
…ily until unit test var bug fix
@davidbooke4 amazing! Just 3 things:
|
@adamribaudo-velir I left the unit tests folder since I wasn't able to migrate the |
…nd delete fixture csv
Oh good call. No harm in leaving it until that bug is resolved. We'll just want to make sure that both this Pytest + the unit tests are running during CI |
…user.yml to gitignore
…w dbt unit test job
…ironment variables instead of project variables
Alright, this is all set for another look whenever you get the chance! |
README.md
Outdated
@@ -302,9 +308,35 @@ The easiest option is using OAuth with your Google Account. Summarized instructi | |||
``` | |||
gcloud auth application-default login --scopes=https://www.googleapis.com/auth/bigquery,https://www.googleapis.com/auth/iam.test | |||
``` | |||
|
|||
The `profiles.yml` file included in this package should be removed. The `profile: 'default'` line in `dbt_project.yml` in this package should also be removed. |
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 own dbt_project.yml
. So models such as stg_ga4__page_conversions
and stg_ga4__derived_session_properties
will be enabled and have fields created based on the variables I've added to dbt_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 those pytest
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.
…ental days environment variable before using project variable
@adamribaudo-velir I also wanted to call out that these unit tests will run when users install this package and execute a |
Thank you both for all of your work on this. I've been watching the conversation for awhile. I just tried running this and it seems that you get a "Parsing Error" if you don't have the This model only gets enabled when you have
I think I should maybe report this as a dbt issue. I've had a number of issues where package YML cause issues when you override at the project level. Introducing this new testing mechanism just makes them worse. I'm thinking as a workaround, we could maybe create an empty View when the variable is not set rather than set |
Thanks for looking this over @dgitis! There's this open issue and PR related to disabling unit tests when their corresponding models are disabled. I was able to compile a project in which I installed this version of the dbt-ga4 package and then set the I'm hopeful that the PR will be resolved soon and this problem will be fixed, but materializing the model as an empty view is an alternative I hadn't considered. I was debating keeping the |
Description & motivation
The goal of this PR is to move the
pytest
unit tests to dbt unit tests.Key Changes
pytest
unit tests to dbt unit testsstg_ga4__events
forurl_parsing
macros which didn't havepytest
unit testsor event_source is null
condition tosession_
fields instg_ga4__sessions_traffic_sources
model. I did this because it seemed that sessions with a nullevent_source
but non-null other properties were incorrectly being assigned(none)
values and attributed to theDirect
default_channel_grouping
.pytest
unit test files for unit tests that have been moved to dbt unit testsrun_unit_tests_on_pr.yml
for executing the dbt unit tests. To support the Github Action that will execute this job, I've done the following:run_unit_tests_on_pr.yml
profiles.yml
file for the Github Action to use to connect to BigQuerydbt_project.yml
base_select.sql
andsrc_ga4.yml
so environment variables are used when thesource_project
andproperty_ids
variables are not set indbt_project.yml
base_ga4__events.sql
so it looks for start date variable from both project and environment variablesREADME.md
related to the unit tests in the packagebase_ga4__events
that checks whether the--empty
flag was passed in the dbt run/build command. Without this, the model fails due to an error because alimit 0
is added tofrom {{ source('ga4', 'events') }}
and the_table_suffix
field is not recognized.Notes
stg_ga4__sessions_traffic_sources_last_non_direct_daily.yml
due to this bug which is preventing migrating thispytest
unit test to a dbt unit test. For the same reason, I've kept thepytest
unit test associated with this model.run_dbt_unit_tests
Github Action can be found here. I had to create a draft PR not from a fork in order to test the changes made inrun_unit_tests_on_pr.yml
.Checklist
dbt test
andpython -m pytest .
to validate existing tests