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

Custom names for generic tests #4898

Merged
merged 8 commits into from
Mar 25, 2022
Merged

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented Mar 18, 2022

resolves #3348 (comment)
resolves #4012

The fuller issue is about rethinking how we construct unique_id. The comment, and this PR, seeks to address the narrower issue that we've seen come up most often in the community (and in #4684, #4102, etc).

TODO

  • Do we want to append the model/source name to the test name? Or trust users to pick globally unique ones?
  • Do people like this proposed syntax?
  • Add integration tests, ideally using the new pytest framework :)

Description

Support this:

models:
  - name: my_model
    columns:
      - name: id
        tests:
          - unique:
              name: my_cool_test
$ dbt test
...
07:43:51  1 of 1 START test my_cool_test.................................................. [RUN]
07:43:51  1 of 1 PASS my_cool_test........................................................ [PASS in 0.03s]

Why this change?

See the linked comment for thorough explanation. Quick answers below:

  • Better control over the name of the test as it appears in console output. Aliasing the database tables was nice, but insufficient
  • Naming generic tests is a step toward documenting tests, until we can support actual test descriptions (Configurable description in yaml config for generic tests #3249)
  • Avoid collisions between generic tests that are identical except for differences in configuration, e.g.

What this PR doesn't do: Alter, in any way, the logic that dbt uses to synthesize generic test names if no custom name is provided. The proposal in #3348 (comment) does include a cleaner more consistent approach to hashed test names. I opted against it, to keep the changes here minimal and additive-only. We can discuss whether that change feels necessary.

While we're here...

Also add support for defining tests as unified dictionaries, like other resource types, in addition to nested dictionaries with the test name as the top-level key. If done in this way, the name of the generic test (unique, not_null, etc) is defined as test_name.

models:
  - name: my_model
    columns:
      - name: id
        tests:
          # these are equivalent
          - accepted_values:
              name: only_allow_abc
              values: ['a', 'b', 'c']
              config:
                severity: warn
                where: 1=1
          - name: only_allow_abc
            test_name: accepted_values
            values: ['a', 'b', 'c']
            config:
              severity: warn
              where: 1=1

I like the second formulation a bit better when defining lots of custom properties and configurations. It's important to continue supporting the first spec; after all, most generic tests are defined with a single keyword (the test name alone).

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 added information about my change to be included in the CHANGELOG.

@cla-bot cla-bot bot added the cla:yes label Mar 18, 2022
@jtcohen6 jtcohen6 changed the title Feature/test custom names alt spec Custom names for generic tests Mar 18, 2022
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

Copy link
Contributor

@nathaniel-may nathaniel-may left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me! Thanks for the very clear description. I have one important thought on the keys we use to define this and a few minor notes. I'm also withholding approval till we get the integration tests in this PR.

[blocker] I don't like the distinction between name and test_name. I'd like to see the functionality of defining unique test names associated with exactly one key regardless of the test its being applied to.

I opted against it, to keep the changes here minimal and additive-only.

I think this is a good approach. The advantage of modifying the internal hashing strategy is that a dbt version upgrade with zero code changes would fix the issue where long test names can't be run, and that can absolutely be done in another PR. We should probably open a clear ticket to that effect so the work doesn't get lost.

test_type: str, test_name: str, args: Dict[str, Any]
) -> Tuple[str, str]:
# Using the type, name, and arguments to this generic test, synthesize a (hopefully) unique name
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] If we're going to use the word "hopefully" in this comment let's lay out exactly when this would not be unique.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for adding that!

self.fqn_name: str = ""

if "name" in self.args:
# TODO: Should we append the model name to the test name here?
Copy link
Contributor

@nathaniel-may nathaniel-may Mar 21, 2022

Choose a reason for hiding this comment

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

[blocker] I believe the result of this name mangling strategy should include the model name so our test-uniqueness constraints can be mechanically enforced if we want them to be.

@@ -401,7 +428,8 @@ def macro_name(self) -> str:
macro_name = "{}.{}".format(self.namespace, macro_name)
return macro_name

def get_test_name(self) -> Tuple[str, str]:
def get_synthetic_test_names(self) -> Tuple[str, str]:
# Returns two names: shorter (for the compiled file), full (for the unique_id + FQN)
Copy link
Contributor

Choose a reason for hiding this comment

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

This type signature is a very easy-to-misuse one. good call adding a comment.

@gshank
Copy link
Contributor

gshank commented Mar 22, 2022

One concern I have is that people may not realize that they've defined an 'only_allow_abc' test in a different file. I'm thinking that we should lookup the test name and see if a test already exists with that name, and if it does issue an error message. Otherwise it feels like the tests could silently overwrite each other (like they sometimes do today). Always suffixing the model name would eliminate a lot of duplicates, but it would still be possible to name two tests on the same model with the same test name.

@jtcohen6
Copy link
Contributor Author

@nathaniel-may @gshank Thank you both for the speedy review + feedback!

Nomenclature

[blocker] I don't like the distinction between name and test_name. I'd like to see the functionality of defining unique test names associated with exactly one key regardless of the test its being applied to.

Could you say a bit more about this?

I agree this nomenclature can be somewhat confusing:

  • name is the name of this specific instance/case of a generic test (i.e. this test node)
  • test_name is the name of the generic test (macro), of which this test (node) is a specific instance

I picked test_name for consistency with how it's called in our selection syntax: https://docs.getdbt.com/reference/node-selection/methods#the-test_name-method

# select all instances of "unique" tests in the project
$ dbt test --select test_name:unique

I'm not sure what you mean by "associated with exactly one key." If I define a test via just the test name (unique)—the most expedient and common type we see—I'll get an auto-generated ("synthetic") name for that specific test case/node, which factors in the model/column names. If I want to give that test case/node a custom name, that's an additional key.

Internal hashing strat

The advantage of modifying the internal hashing strategy is that a dbt version upgrade with zero code changes would fix the issue where long test names can't be run, and that can absolutely be done in another PR. We should probably open a clear ticket to that effect so the work doesn't get lost.

Agree! I think that ticket could be spun off #4684. FWIW our synthetic naming strategy is already meant to account for too-long test names, by hashing and truncating names longer than 64 characters. That's the motivation behind the short_name, full_name tuple situation. So that approach must be breaking down somewhere along the way.

The plus of such a fix would be, it works for users with zero code changes on their end. The downside would be, we potentially "reset" synthetic test names for lots of folks, causing a break in any metadata tracking. Very much worth it IMO, just something we'd want to include in a minor (not patch) release.

How should uniqueness work?

[blocker] I believe the result of this name mangling strategy should include the model name so our test-uniqueness constraints can be mechanically enforced if we want them to be.

Always suffixing the model name would eliminate a lot of duplicates, but it would still be possible to name two tests on the same model with the same test name.

It turns out, this actually works:

version: 2

models:
  - name: model_a
    columns:
      - name: id
        tests:
          - unique:
              name: my_not_so_unique_test_name
  - name: model_b
    columns:
      - name: id
        tests:
          - unique:
              name: my_not_so_unique_test_name
$ dbt test
09:24:42  Running with dbt=1.0.1
09:24:43  Found 2 models, 2 tests, 0 snapshots, 0 analyses, 367 macros, 0 operations, 1 seed file, 0 sources, 0 exposures, 0 metrics
09:24:43
09:24:43  Concurrency: 5 threads (target='dev')
09:24:43
09:24:43  1 of 2 START test my_definitely_unique_test_name................................ [RUN]
09:24:43  2 of 2 START test my_definitely_unique_test_name................................ [RUN]
09:24:43  1 of 2 PASS my_definitely_unique_test_name...................................... [PASS in 0.04s]
09:24:43  2 of 2 PASS my_definitely_unique_test_name...................................... [PASS in 0.04s]
09:24:43
09:24:43  Finished running 2 tests in 0.17s.
09:24:43
09:24:43  Completed successfully
09:24:43
09:24:43  Done. PASS=2 WARN=0 ERROR=0 SKIP=0 TOTAL=2

Why? Because of the hash that we append to each test's unique_id, since #3335, which does include the model name (and column name, and other arguments).

$ dbt ls -s my_not_so_unique_test_name --output json --output-keys name,unique_id
{"unique_id": "test.testy.my_not_so_unique_test_name.116ca7b894", "name": "my_not_so_unique_test_name"}
{"unique_id": "test.testy.my_not_so_unique_test_name.a6184f01da", "name": "my_not_so_unique_test_name"}

So the tests are disambiguated within dbt internally, just as they would be if the model name were prepended to the name. For the end user, it might be a bit confusing, since dbt test --select my_not_so_unique_test_name actually returns two tests... but that's about it.

The only way to get actual dupes is by reproducing the setup from #4102, where the same generic test is defined on the same model + column, and the only thing different is the config (not factored into the hash, for all the reasons we've discussed):

models:
  - name: model_a
    columns:
      - name: id
        tests:
          - unique:
              name: my_not_so_unique_test_name
              where: 1=1
          - unique:
              name: my_not_so_unique_test_name
              where: 1=2
Compilation Error
  dbt found two resources with the name "my_not_so_unique_test_name". Since these resources have the same name,
  dbt will be unable to find the correct resource when "my_not_so_unique_test_name" is used. To fix this,
  change the name of one of these resources:
  - test.testy.my_not_so_unique_test_name.116ca7b894 (models/one_file.yml)
  - test.testy.my_not_so_unique_test_name.116ca7b894 (models/one_file.yml)

The resolution there is to pick some actually unique test names. Those definitions are necessarily in the same file, defined on the same column, so they really cannot be more than a few lines apart from one another.

I'm inclined to prefer keeping it this way, to let users actually set the exact test names they want?

@gshank
Copy link
Contributor

gshank commented Mar 23, 2022

I didn't actually realize that we would still be appending the hash to the unique id. I think that's fine then. Will need documenting of course :)

I do really like the idea of letting people name their tests.

@nathaniel-may
Copy link
Contributor

RE: How should uniqueness work

I love this compilation error at the bottom of your comment. What a clear development experience. I definitely want to make sure we never silently override tests and it sounds like this would prevent that.

If we're going to apply the uniqueness constraint to {model, test_name, column pairs} instead of just {model, test_name} I think we need to display that in the error which is why I don't think hashing in name mangling is a good practice. If you have 1599 columns in your table, it should be simple to see where the conflict is if every column has a test with this name and one test accidentally got copy-pasted twice for a column.

to be concrete, I'd rather see:

...
  - test.testy.model_a.id.my_not_so_unique_test_name (models/one_file.yml)
  - test.testy.model_a.id.my_not_so_unique_test_name (models/one_file.yml)

RE: Nomenclature

I think I totally misunderstood your proposal. My previous comment was under the assumption that sometimes you'd use name to apply your unique name to a test and sometimes you would use test_name. After re-reading your example more carefully it looks like you want to use name every time for that which sounds good to me.

RE: Internal hashing strat

Yeah that's fair, the max length bit might just need to be looked into independently. I know windows enforces a max path length of 255 characters so with longer-named directory structures, a 64 character file could easily overflow that.

@jtcohen6 jtcohen6 force-pushed the feature/test-custom-names-alt-spec branch from b220819 to 75b2f43 Compare March 24, 2022 13:01
@jtcohen6
Copy link
Contributor Author

Functional testing

Lucky for me, these just got converted! #4826

I tried my hand at adding my functional tests. My first go in the new framework. Three test cases:

  • Check that tests collide when only configs differ
  • Ensure that they don't collide when given custom names
  • Ensure that they can be written in the alt format proposed above / in this PR

RE: How should uniqueness work

to be concrete, I'd rather see:

...
 - test.testy.model_a.id.my_not_so_unique_test_name (models/one_file.yml)

Totally fair call on the desirable end state. This message comes from exceptions.raise_duplicate_resource_name, which is used for all resource types (not just tests). By plumbing through the code here, I realized that this is where we used to silently skip the error for duplicate schema tests, and overwrite it in the manifest...

elif node_1.resource_type == NodeType.Test and "schema" in node_1.tags:
return

I like the current behavior a lot more!

Given that the error message already has resource-specific behavior, I've taken a crack at rewriting it to say something more helpful for tests:

Compilation Error
  dbt found two tests with the name "my_not_so_unique_column_level_test" defined on column "id" in "model_a".
  Since these resources have the same name, dbt will be unable to find the correct resource
  when running tests.

  To fix this, change the name of one of these resources:
  - test.testy.my_not_so_unique_column_level_test.116ca7b894 (models/one_file.yml)
  - test.testy.my_not_so_unique_column_level_test.116ca7b894 (models/one_file.yml)
Compilation Error
  dbt found two tests with the name "my_not_so_unique_model_level_test" defined on "model_a".
  Since these resources have the same name, dbt will be unable to find the correct resource
  when running tests.

  To fix this, change the name of one of these resources:
  - test.testy.my_not_so_unique_model_level_test.f9cb21de76 (models/one_file.yml)
  - test.testy.my_not_so_unique_model_level_test.f9cb21de76 (models/one_file.yml)

Why did I rework the error message like this, rather than opting to show test.testy.model_a.id.my_not_so_unique_test_name? What we're showing in the message is each node's unique_id. In the standard method, used across the board for all resource types, this is <resource_type>.<project_name>.<resource_name>. For generic tests in particular, we also added a hash of test metadata (#3335), to solve for 99% of the uniqueness/collision problems we've been discussing. (As we know, we can't reliably solve for 100%.)

So, I don't think we should add the model name (and column name, if relevant) to the test's unique_id. It's totally reasonable to think about adding these to the test's FQN (fully qualified name), which is used for node selection and configuration purposes. (Generally, this is the file path to where the node is defined.) We redefined FQNs for tests ahead of v1.0 (#3880), such that it now includes the path components for the .yml file where the generic test is defined. In that same PR, I'd played around with including the model name and column name in the FQN—it's totally desirable to be able to select/configure all tests defined on X column of Y model—but I think (IIRC) it led to all sorts of surprising behavior around test selection, and a bunch of failing unit/integration tests. I abandoned the change then, since it was out of scope of the actual effort (renaming test types).

@jtcohen6 jtcohen6 marked this pull request as ready for review March 24, 2022 13:02
@jtcohen6 jtcohen6 requested review from a team as code owners March 24, 2022 13:02
elif node_type == NodeType.Test and hasattr(node_1, "test_metadata"):
column_name = f'column "{node_1.column_name}" in ' if node_1.column_name else ""
# TODO: we need a better way of storing the target model/source/etc in generic tests
model_name = ".".join((node_1.refs or node_1.sources)[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this would help, but I did add 'file_key_name' to ParsedGenericTestNode because I was having the same problem in partial parsing. It stores things like 'models.my_model' and 'sources.my_source'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great call! This is much less hacky, so I think it's the right way to go. I do wish the file_key_name for sources stored sources.my_source.my_src_table.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I was thinking you might want that. It might be possible to update it to do that...

Copy link
Contributor

@nathaniel-may nathaniel-may left a comment

Choose a reason for hiding this comment

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

Great work here! I left many nits, but no blockers.

Thanks for clarifying in the previous comment that the output line is the node's unique_id. I do think that is what should actually be changed in an ideal world, but the way we're doing it now works and has less far-reaching implications.

@@ -0,0 +1,7 @@
kind: Features
body: Support custom names for generic tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it just generic tests or all tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Singular tests take their name from the file in which they're defined (tests/my_custom_test.sql), just like models — so they've always required custom names

Copy link
Contributor

Choose a reason for hiding this comment

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

Even more evidence that this PR is a good idea!

Comment on lines 873 to 880
f'dbt found two {pluralized} with the name "{duped_name}". '
f"\n"
f"\nSince these resources have the same name, dbt will be unable to find the correct resource "
f"\nwhen {action} {formatted_name}. "
f"\n"
f"\nTo fix this, change the name of one of these resources: "
f"\n- {node_1.unique_id} ({node_1.original_file_path}) "
f"\n- {node_2.unique_id} ({node_2.original_file_path})"
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] for readability, I'd rather see a multiline f-string instead of relying on c-style string concatenation with explicit newline characters. For example:

f'''line one of {message_noun}
line two of {message_noun}
etc.'''

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The downside of a multiline f-string is that indentation gets messy:

$ dbt test
10:16:41  Running with dbt=1.1.0-b1
10:16:41  Encountered an error:
Compilation Error
  dbt found two tests with the name "my_not_so_unique_column_level_test" defined on column "id" in "sources.my_src".
          Since these resources have the same name, dbt will be unable to find the correct resource
          when running tests.
          To fix this, change the name of one of these resources:

          - test.testy.my_not_so_unique_column_level_test.345c7a643d (models/one_file.yml)

          - test.testy.my_not_so_unique_column_level_test.345c7a643d (models/one_file.yml)

There is a way to get the indentation right, but then it looks a bit odd in the source code:

    raise_compiler_error(f"""
dbt found two {pluralized} with the name "{duped_name}".

Since these resources have the same name, dbt will be unable to find the correct resource
when {action} {formatted_name}.

To fix this, change the name of one of these resources:
- {node_1.unique_id} ({node_1.original_file_path})
- {node_2.unique_id} ({node_2.original_file_path})
    """.strip())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

flake + black seem to be happy with the latter, so I think that's what I'll go with

Copy link
Contributor

Choose a reason for hiding this comment

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

yup. this is exactly the trade off, but I most commonly see the latter across languages that support "raw strings" so I'm inclined to do it here as well.

test_type: str, test_name: str, args: Dict[str, Any]
) -> Tuple[str, str]:
# Using the type, name, and arguments to this generic test, synthesize a (hopefully) unique name
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for adding that!

# duplicate generic tests
elif node_type == NodeType.Test and hasattr(node_1, "test_metadata"):
column_name = f'column "{node_1.column_name}" in ' if node_1.column_name else ""
# TODO: we need a better way of storing the target model/source/etc in generic tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's log a tech debt ticket and put the github issue number in the comment here, unless @gshank's suggestion is the solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gerda's suggestion is definitely better than what I'm doing here, and I think it's enough for now—but I agree that this is a limitation in generic tests, which also prevents us from being able to do things like #4723

self.fqn_name: str = ""

if "name" in self.args:
# Trust the user to have a unique name for this model + column combo
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] "trust the user" makes it sounds like we're relying on them to enforce uniqueness themselves. We aren't because we raise an error. I'd change this comment to something more like "assign the user-defined name here, but it will be checked for uniqueness later"

test_name, test_args = test[0]
# If the test is a dictionary with top-level keys, the test name is "test_name"
# and the rest are arguments
# {'name': 'my_favorite_test', 'test_name': 'unique', 'config': {'where': '1=1'}}
Copy link
Contributor

Choose a reason for hiding this comment

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

test_name and test name are confusing, so this example really helps 👍

@@ -658,6 +661,60 @@ def test_collision_test_names_get_hash(
assert test_results[1].node.unique_id in expected_unique_ids


class TestGenericTestsCollide:
@pytest.fixture(scope="class")
def models(self, dupe_tests_collide): # noqa: F811
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] # noqa: F811 is a little unexpected here since that's usually for import statements. Does it work without this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this from the other test definitions, it looks like this is a flake8 complaint:

tests/functional/schema_tests/test_schema_v2_tests.py:666:22: F811 redefinition of unused 'dupe_tests_collide' from line 6

Indeed because dupe_tests_collide is being imported at the top, from the separate file of fixtures (tests.functional.schema_tests.fixtures)

Copy link
Contributor

Choose a reason for hiding this comment

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

interesting. that seems like a flake8 bug counting a reference as a redefinition. thanks for double checking!

def models(self, custom_generic_test_names): # noqa: F811
return custom_generic_test_names

def test_collision_test_names_get_hash(
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] let's add a comment to all the test methods describing why we need this test. (imagine the reader is considering deleting it)

@nathaniel-may
Copy link
Contributor

Thanks for addressing all the feedback! Looks like a stellar PR to me.

@jtcohen6 jtcohen6 merged commit 5071b00 into main Mar 25, 2022
@jtcohen6 jtcohen6 deleted the feature/test-custom-names-alt-spec branch March 25, 2022 16:09
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.

Large projects see significant slowdown in resolve_graph using v0.21.0 Better node identification
3 participants