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

[CT-2686] Incorrectly set up tests in test_query_comment.py #7845

Closed
2 tasks done
damian3031 opened this issue Jun 12, 2023 · 1 comment · Fixed by #7928
Closed
2 tasks done

[CT-2686] Incorrectly set up tests in test_query_comment.py #7845

damian3031 opened this issue Jun 12, 2023 · 1 comment · Fixed by #7928
Labels
bug Something isn't working help_wanted Trickier changes, with a clear starting point, good for previous/experienced contributors Team:Adapters Issues designated for the adapter area of the code

Comments

@damian3031
Copy link
Contributor

Is this a new bug in dbt-core?

  • I believe this is a new bug in dbt-core
  • I have searched the existing issues, and I could not find an existing issue for this bug

Current Behavior

Tests in test_query_comment.py are incorrectly set up. They are always passing, no matter if query comments are correctly implemented.

Assertions are placed in functions named matches_comment. As pytest run functions with names prefixed with test, these assertions are never checked.

The only thing which this tests are doing, is invoking test_comments function, which invokes run_assert_comments, which invokes run_get_json, which is capturing logs. There are no assertions in any of above functions.

Additionally, I think that it would be better to check in dbt_run result query if query-comments were added, instead of checking in logs.

Expected Behavior

Tests should detect bugs instead of always pass.

Steps To Reproduce

  1. In test_query_comment.py Change any assertion from any matches_comment, to wrong assertion e.g. assert 0 == 1. Run tests, notice that they are still passing.
  2. Add some wrong assertion in test_comments function. Run tests, notice that all tests from this file are failing

Relevant log output

No response

Environment

- OS: macOS
- Python: 3.10
- dbt: v1.6.0b3

Which database adapter are you using with dbt?

postgres

Additional Context

I'm willing to create PR for this issue, as I already overwrote these tests in dbt-trino to test query-comments properly on this adapter.

@damian3031 damian3031 added bug Something isn't working triage labels Jun 12, 2023
@github-actions github-actions bot changed the title Incorrectly set up tests in test_query_comment.py [CT-2686] Incorrectly set up tests in test_query_comment.py Jun 12, 2023
@dbeatty10
Copy link
Contributor

Thank you for noticing and reporting this @damian3031 🏆

That would be awesome if you are willing to raise a PR to fix this. When you open a PR for this, could you tag @dataders ? He won't be the final reviewer, but he can give initial feedback and help get it into our PR review queue.

@dbeatty10 dbeatty10 added help_wanted Trickier changes, with a clear starting point, good for previous/experienced contributors Team:Adapters Issues designated for the adapter area of the code and removed triage labels Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help_wanted Trickier changes, with a clear starting point, good for previous/experienced contributors Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants