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.

Sign up for GitHub

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

Jump to bottom

Rename tests: singular + generic #3880

Merged
merged 6 commits into from
Sep 30, 2021
Merged

Rename tests: singular + generic #3880

merged 6 commits into from
Sep 30, 2021

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented Sep 14, 2021 •
edited
Loading

resolves #3234, #3259, #3347

Basics

  • Rename data → singular, schema → generic
  • Retain backwards compatibility for test_type:schema + test_type:data

Opinionated choices

  • ❗ Rework FQN construction: Exclude test type signifier (schema_test/data_test). Singular test FQNs are constructed exactly the same as models, by relative path (excluding top-level project folder) and name. Generic test FQNs include relative path to the yaml file that defines them.
    • This makes it possible to configure subsets of generic tests by subdirectory!
    • The compiled_path will also no longer include the test type prefix (schema_test/, data_test/). I removed this because it seems irrelevant, and has no real functional effect.
  • ❗ ❗ Remove support for --data and --schema flags to dbt test entirely. Remove data + schema tags from tests. This will be a breaking change for users who are still relying on the old way of doing this, before we released the test_type: selection method in v0.18. I put this into a separate commit, so we can revert if we don't agree.

Comments

  • Planning to include this in v1.0.0-b1, rather than rushing to include in v0.21. Let me know if you disagree.
  • Confirmed that I need to fix Schema tests for sources are added to the manifest twice. #3347 in order to get tests passing. I don't know why that issue only starts causing functional problems with the code in this PR; it sure seems like it should be causing the same issue today. I made the fix as a standalone commit, let me know if I should pull into a separate PR.
  • Since I'm already touching test FQNs, why not make them even better? (Improve test FQNs #3259, probably out of scope here, though sort of a shame to leave them as they are in v1.0) I took a swing at these. Will add some comments above.
  • I renamed file names containing schema_test or data_test when they are dbt-core modules. I'd prefer to not rename integration tests for now, to avoid big git diffs.

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 Sep 14, 2021
@jtcohen6 jtcohen6 marked this pull request as ready for review September 14, 2021 10:56
leahwicz
leahwicz reviewed Sep 15, 2021
View reviewed changes
core/dbt/parser/schemas.py
@@ -325,8 +325,6 @@ def _parse_generic_test(
'kwargs': builder.args,
}
tags = sorted(set(itertools.chain(tags, builder.tags())))
if 'schema' not in tags:
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems randomly removed but maybe it isn't?

Copy link
Contributor Author

@jtcohen6 jtcohen6 Sep 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Super fair question!

In v0.18, we introduced a selection method, test_type:, that allowed users to select just schema tests (dbt test --models test_type:schema) or data tests (dbt test --models test_type:data).

Prior to that, we supported the same functionality, but in a pretty silly way. dbt would secretly add tags—'schema' to schema tests, 'data' to data tests—and then users could pass a flag dbt test --schema to select all tests tagged "schema," or dbt test --data to select all tests tagged "data." Funny enough, that would include schema tests manually tagged "data," too, or data tests that selected from models tagged "schema." Not a good system. We haven't explicitly said that the older system will stop working in a future version, but we effectively replaced it with the test_type method in the documentation (e.g. the changelog note here).

This PR takes the opinionated view that we should do away with that legacy approach to the functionality, since we've had a better approach (test_type selector) for several versions now. To that end, there's some backwards compatibility for the newer approach, and total deprecation fro the older approach:

  • Stop secretly adding 'schema' and 'data' tags to tests
  • Deprecate --schema and --data flags entirely
  • Continue supporting test_type:schema + test_type:data as aliases for test_type:generic and test_type:singular, respectively

leahwicz
leahwicz reviewed Sep 15, 2021
View reviewed changes
core/dbt/parser/sources.py
@@ -269,11 +269,6 @@ def parse_source_test(
tags=tags,
column_name=column_name
)
# we can't go through result.add_node - no file... instead!
if node.config.enabled:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. What does this have to do with the rename? Honestly asking to learn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the fix for #3347, in which we noticed a while ago that source tests are added to the manifest twice.

Things I don't know:

  • why that doesn't cause a functional error currently
  • why that started causing a functional error when I made the other changes in this PR, i.e. renamed things

Despite not knowing those things, I feel reasonably good about making this fix. I can move the commit (0adec32) into a separate PR if that would make things clearer.

@jtcohen6 jtcohen6 mentioned this pull request Sep 24, 2021
Closed
@jtcohen6 jtcohen6 force-pushed the rename/singular-generic-tests branch 2 times, most recently from 4aac13f to 8e66df3 Compare September 26, 2021 17:06
gshank
gshank approved these changes Sep 29, 2021
View reviewed changes
Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

Looks good!

@jtcohen6 jtcohen6 merged commit ca7c1fc into develop Sep 30, 2021
@jtcohen6 jtcohen6 deleted the rename/singular-generic-tests branch September 30, 2021 13:57
@jtcohen6 jtcohen6 mentioned this pull request Sep 30, 2021
Closed
jtcohen6 added a commit to dbt-labs/dbt-redshift that referenced this pull request Oct 2, 2021
@jtcohen6
Test rename dbt-labs/dbt-core#3880
92aa96d
jtcohen6 added a commit to dbt-labs/dbt-redshift that referenced this pull request Oct 2, 2021
@jtcohen6
Test rename dbt-labs/dbt-core#3880
ae34880
jtcohen6 added a commit to dbt-labs/dbt-redshift that referenced this pull request Oct 2, 2021
@jtcohen6
Test rename dbt-labs/dbt-core#3880
1e0a38e
@jtcohen6 jtcohen6 mentioned this pull request Nov 8, 2021
Closed
iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
@jtcohen6 @iknox-fa
Rename tests: singular + generic (#3880)
b76bbca 
* Fix #3347

* Rename data->singular, schema->generic

* Rm schema, data flag + tag behavior

* Update test FQNs, compiled paths

* Add changelog entry

automatic commit by git-black, original commits:
  ca7c1fc
iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
@jtcohen6 @iknox-fa
Rename tests: singular + generic (#3880)
df74fa0 
* Fix #3347

* Rename data->singular, schema->generic

* Rm schema, data flag + tag behavior

* Update test FQNs, compiled paths

* Add changelog entry

automatic commit by git-black, original commits:
  a5ce658
  ca7c1fc
@jtcohen6 jtcohen6 mentioned this pull request Mar 24, 2022
Merged
7 tasks
@savannahcarnahan savannahcarnahan mentioned this pull request Sep 27, 2022
Merged
runleonarun added a commit to dbt-labs/docs.getdbt.com that referenced this pull request May 25, 2023
@savannahcarnahan @mirnawong1 @runleonarun
Update test-selection-examples.md (#2090)
302f91f 
Updated syntax from legacy test_type name (data) to current nomenclature
(singular)

## Description & motivation
<!---
Describe your changes, and why you're making them. Is this linked to an
open
issue, a pull request on dbt core, etc?

To learn more about the writing conventions used in the dbt Labs docs,
see the [Content style
guide](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/content-style-guide.md).
-->
I was confused by the test_type:data designation under Test Selection:
Examples. I was able to find a pull request documenting the legacy
test_type tags [here](dbt-labs/dbt-core#3880). I
used this to update the example for the current version.

Co-authored-by: mirnawong1 <[email protected]>
Co-authored-by: Leona B. Campbell <[email protected]>
abbywh pushed a commit to abbywh/dbt-redshift that referenced this pull request Oct 11, 2023
@jtcohen6
Test rename dbt-labs/dbt-core#3880
867f0fc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gshank gshank gshank approved these changes

@kwigley kwigley Awaiting requested review from kwigley

@leahwicz leahwicz Awaiting requested review from leahwicz

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Schema tests for sources are added to the manifest twice. Rename "schema" + "data" tests
3 participants
@jtcohen6 @gshank @leahwicz