load_from_dbt_manifest is de-selecting valid test nodes

[adammarples]

NodeSelector._should_include_node has the following line

if node.resource_type == DbtResourceType.TEST:
    node.tags = getattr(self.nodes.get(node.depends_on[0]), "tags", [])

which gives the test node the tags of its parent model. Obviously this means that we cannot select tests by tag as we can in dbt-core, which would be ideal, but there is another problem.

node.depends_on[0] is supposed to return the id of the parent model of the test, however, it actually just selects the first dependency. We have tests with additional dependencies (sources), ie.

models:
- name: MY_MODEL
  config:
    tags:
    - mytag
  tests:
  - comparison_to_expected_results:
      expected: source('EXPECTED', 'MY_MODEL')

This is valid dbt but now breaks in astronomer-cosmos because the test tags are set to the source tags, not the parent model tags.

Solution:

I have found that node_dict['refs'][0]['name'] is more reliable for fetching the parent model of the test than node_dict.get("depends_on", {}).get("nodes", []) which is used in the DbtNode class, although I appreciate that dbt doesn't seem to offer a reliable way to do this.

Alternatively, we could try and remove non-model ids from the list at least to try and prune it down to being more likely to be the parent model rather than a stray source, and warn on lists greater than length 1.

if node.resource_type == DbtResourceType.TEST:
    dependency_ids = [node_id for node_id in node.depends_on if node_id.startswith('model.')]
    parent_id = dependency_ids[-1]
    if len(dependency_ids) > 1:
        logger.warning(
            f'Test node {node.name} has more than one model dependency {dependency_ids}, selected tags from parent assumed to be {parent_id}.')
    parent_tags = getattr(self.nodes.get(parent_id), "tags", [])
    node.tags = list(set(node.tags + parent_tags))

Here we can additionally use and filter on the actual test tags too, improving parity with dbt-core --select.

Alternatively, I secretly suspect that depends_on['nodes'][-1] will always be the test parent model, instead of depends_on['nodes'][0], although I haven't been through the dbt-core code to prove this.

[dosubot]

🤖

Thank you for bringing this issue to our attention and proposing a solution! Your analysis of the problem and the suggested code changes seem reasonable.

Encouraging users to contribute to open-source projects like Cosmos is always appreciated. If you are interested in contributing, I would encourage you to create a pull request with your proposed changes. The maintainers will review your changes and determine if they should be merged into the project.

Keep up the great work and thank you for your support of Cosmos!

[adammarples]

#751

[tatiana]

Update: we've advanced in discussing this ticket.

Cosmos should improve its behaviour to consider multiple parents so the test node selection is compatible with how dbt does this:
#751 (comment)

The workaround is to use ExecutionConfig.test_indirect_selection, as suggested by @jbandoro : #751 (comment)

[dosubot]

Hi, @adammarples,

I'm helping the Cosmos team manage their backlog and am marking this issue as stale. The issue "load_from_dbt_manifest" was causing valid test nodes to be deselected due to a bug in the "NodeSelector._should_include_node" method. The problem arose from test nodes inheriting tags from their parent model, making them unable to be selected by tag. The issue has been resolved with a proposed solution involving using a more reliable method to fetch the parent model of the test and filtering out non-model dependencies. There is an open pull request for proposed changes, and the discussion is ongoing regarding improving Cosmos' behavior to consider multiple parents for test node selection.

Could you please confirm if this issue is still relevant to the latest version of the Cosmos repository? If it is, please let the Cosmos team know by commenting on the issue. Otherwise, feel free to close the issue yourself, or the issue will be automatically closed in 7 days.

Thank you!