[CT-1271] Test Query Comment conversion #5971
Conversation
…ts passing locally for core
|
Few open questions for discussion:
|
tests/adapter/dbt/tests/adapter/query_comment/test_query_comment.py
Outdated
Show resolved
Hide resolved
tests/adapter/dbt/tests/adapter/query_comment/test_query_comment.py
Outdated
Show resolved
Hide resolved
tests/adapter/dbt/tests/adapter/query_comment/test_query_comment.py
Outdated
Show resolved
Hide resolved
| {% set required = ['name', 'schema', 'type', 'threads'] %} | ||
|
|
||
| {# Require what we docuement at https://docs.getdbt.com/docs/target #} | ||
| {% if target.type == 'postgres' or target.type == 'redshift' %} |
There was a problem hiding this comment.
The problem with doing it this way is that it doesn't support external adapter maintainers very well, and we don't actually provide an example of how to get this working with an external adapter except by replacing this entire model. In addition, putting the test in jinja model code is kind of a bad pattern.
It would be better to put the required fields in some kind of config that can be retrieved. Also, it would probably be better to move the checks into the actual test code, rather than embedding it in the model. I'd make the "required" fields a fixture that can be overridden in the adapter specific tests, and then pull that into the test method and test for the existence of those fields in the target in the test. I'm not exactly sure how you can get the target dictionary. Possibly from the adapter? I can help look for how to get that if you need.
There was a problem hiding this comment.
I'm not even sure why we're doing this in this test. What does this have to do with query comments?
There was a problem hiding this comment.
as we talked about i'm a little hazy on why this is here other than validation logic possibly against if we did import it to a different database to make sure its not actually relying off say postgres.
would love to find a better way to work on this will play around some of these other options if we do think this chucnk is needed at all.
There was a problem hiding this comment.
Hey @gerda had a few questions/ideas are we able to set up some global stuff for checks in a fixture that can't be overwritten by chance? like the
{% set required = ['name', 'schema', 'type', 'threads'] %} that might not change between the adapters?
trying to get ahold of the related information that would be same as target in the fixture looks like from our TestProjInfo something like profiles_dir would contain the information we would want just not sure if theres a good way to tap into that possibly by some os read stuff?
| if seen: | ||
| "Never saw a matching log message! Logs:\n{}".format("\n".join(log["msg"])) | ||
|
|
||
| def test_comments(self, project): |
There was a problem hiding this comment.
I don't see a reason for just forwarding to another method here. Just
There was a problem hiding this comment.
just? @gshank did something get cutoff here?
There was a problem hiding this comment.
maybe? Probably was saying "just call the method directly"
resolves #5962
issue: #6272 (comment)
Description:
converting integration test 051 to a new functional test
todo:
dbt-coreand all required adapters including:dbt-sparkChecklist
changie newto create a changelog entry