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 the tests (#2317) #2318

Merged
merged 3 commits into from Apr 14, 2020
Merged

Fix the tests (#2317) #2318

merged 3 commits into from Apr 14, 2020

Conversation

beckjake
Copy link
Contributor

resolves #2317

Description

This looks like a neat interplay between yaml's anchor/pointer notation (it's
a reference, not a copy!), and dbt inserting 'model' into the keyword args for
tests.

I've added a copy.deepcopy call to where dbt collects test arguments from the
underlying file, so the 'model' is inserted into a fresh keyword arguments
dictionary.

While I was in there, I also removed a method that's not called anymore.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR The tests were already there!
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section. - I don't think this merits a CHANGELOG update?

This looks like a neat interplay between yaml's anchor/pointer notation (it's
a reference, not a copy!), and dbt inserting 'model' into the keyword args for
tests.

I've added a `copy.deepcopy` call to where dbt collects test arguments from the
underlying file, so the 'model' is inserted into a fresh keyword arguments
dictionary.
@cla-bot cla-bot bot added the cla:yes label Apr 14, 2020
Since they apparently will do this in patch releases, pinnned to precisely 2.11.2
But, it means we don't have to override NativeSandboxTemplate.render
anymore, so that's nice
@beckjake
Copy link
Contributor Author

These tests broke because Jinja2 removed a keyword argument that we used to override render (in a patch release! see pallets/jinja#1190).

I've accordingly locked Jinja's version to 2.11.2, unfortunately it looks like we can't count on it to provide a stable API.

Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

LGTM :)

@beckjake beckjake merged commit 6424b65 into dev/octavius-catto Apr 14, 2020
@beckjake beckjake deleted the fix/bq-dp-models-failure branch April 14, 2020 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix the build in dev/octavius-catto
2 participants