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

convert tests #2684

Merged
merged 1 commit into from
Aug 5, 2020
Merged

Conversation

beckjake
Copy link
Contributor

@beckjake beckjake commented Aug 4, 2020

resolves #2415

requires #2679

Description

The existing tests all still pass (a good start!). Locally I modified the postgres adapter to add an always-failing postgres__test_not_null and everything worked.

We don't really have a good way to write automated tests for adapter plugin behavior itself, unfortunately. I think the general case of it will be pretty hard to solve!

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
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot cla-bot bot added the cla:yes label Aug 4, 2020
Base automatically changed from feature/adapter-dispatch to dev/marian-anderson August 4, 2020 18:01
@beckjake beckjake marked this pull request as ready for review August 4, 2020 18:01
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Cool that all the tests pass as-is! This worked for me locally, too. I was able to create a version of dbt-spark that overrode the unique test by defining spark__test_unique.

This override is only possible if the test is defined in a plugin (using dbt namespace) or in the macro-paths of a user's root project. It will not work if [prefix]__test_unique is defined in an installed package. That feels right. (I suppose the package could still just override test_unique, though multiple installed packages overriding the same-named macro gets tricky, right?)

We don't really have a good way to write automated tests for adapter plugin behavior itself, unfortunately.

Do you mean the behavior of an hypothetical adapter plugin, confirming that it has the ability to override these macros?

@beckjake
Copy link
Contributor Author

beckjake commented Aug 4, 2020

This override is only possible if the test is defined in a plugin (using dbt namespace) or in the macro-paths of a user's root project. It will not work if [prefix]__test_unique is defined in an installed package. That feels right.

Huh, I don't really expect that! I will see if that behavior is fixed when I'm done with #2301 since that behavior feels like the most likely place this behavior would slip in.

(I suppose the package could still just override test_unique, though multiple installed packages overriding the same-named macro gets tricky, right?)

Yeah, not really recommended! I think dbt will just pick whatever shows up first. That would depend on... I don't even know. Inode order of the directories in dbt_modules?

Do you mean the behavior of an hypothetical adapter plugin, confirming that it has the ability to override these macros?

Yes.

@beckjake
Copy link
Contributor Author

beckjake commented Aug 5, 2020

Coming back to this, I have come to accept that the behavior of macro lookups is correct as a consequence of how macros do their namespacing. macros in non-root non-"dbt" namespaces do not appear in the default namespace, so they shouldn't show up as tests.

You can access a dependency's test overrides by using dep_name.unique as your test, though it won't pick up the adapter macro behavior, so you have to name your dependency's test just test_unique instead of default__test_unique (or use adapter.dispatch and write both!).

I think this whole thing is a little more confusing than I'd really like, but the fix for that is probably for tests to be more like materializations: a custom jinja block that specifies the adapter name in its definition. Good news, there's already an issue for that somewhere!

@beckjake beckjake merged commit e641ec1 into dev/marian-anderson Aug 5, 2020
@beckjake beckjake deleted the feature/dispatch-schema-tests branch August 5, 2020 15:28
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.

Use adapter macro pattern for core schema tests
3 participants