Ct 1517/convert 060 persist docs #6409
Conversation
|
If I understand the intent correctly, we're moving all of the files into fixtures. So instead of moving the .sql/.yml files from integration to functional, shouldn't we just delete them? From spot checking, it looks like we already put those in |
|
|
||
| class BasePersistDocsTest: | ||
| @pytest.fixture(scope="class", autouse=True) | ||
| def setUp(self, project): |
There was a problem hiding this comment.
I don't think this is a fixture, is it? If you want this to run once per class, changing it to this should work:
@classmethod
def setup_class(cls):
...
If you need it for each method, then this should work:
def setup_method(self, method):
...
There was a problem hiding this comment.
We're using fixtures and these methods are for the "classical" way of doing tests. So, if I'm not mistaken, those aren't going to mesh well with our current framework.
Instead, we have an adhoc setup fixture we sometimes include. Granted, there's a few ways to do this depending on your use case. Sometimes, I define the class setup fixture at modular level and sometimes class scope it. Just depends.
For more, check out this SO post.
| @@ -40,8 +70,12 @@ def _assert_has_table_comments(self, table_node): | |||
| table_comment, table_id_comment, table_name_comment | |||
| ) | |||
|
|
|||
| def _assert_has_view_comments(self, view_node, has_node_comments=True, | |||
| has_column_comments=True): | |||
| def _assert_has_view_comments( | |||
There was a problem hiding this comment.
This and _assert_has_table_comments() look like a good use case for parameterized tests (@pytest.mark.paraterize(...). I think that's what's effectively happening here, but it could be maintained more easily with the pytest built-ins.
There was a problem hiding this comment.
I looked into this, and agree it's a neat built in. But I'm not sure it matches this use case. This flag appears to accomodate fixed values whereas we're passing in dynamically generated nodes. Am I missing something from your thinking here?
|
Yep, you're right on those old files. They were added with a sloppy git add. I like how the one time I use -a, the reason you shouldn't use -a happens. |
resolves #6273
Description
Convert the test. There was a seed in here unused for whatever reason. I added it into the code env and bumped the assertion rhs in a line to complete the re-add.
Checklist