Snapshot strategies: newline for subquery

[RobbertDM]

resolves #6781

Description

If you don't add these, the ) subq will be appended to the last line of the snapshot query, which could be a comment line.

Checklist

• I have read the contributing guide and understand what's expected of me
• 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 opened an issue to add/update docs, or docs changes are not required/relevant for this PR
• I have run changie new to create a changelog entry

[cla-bot]

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR.

CLA has not been signed by users: @RobbertDM

[RobbertDM]

CLA: signed again with case sensitive GitHub username this time 🤔

[dbeatty10]

Thanks for opening this PR @RobbertDM !

A bookkeeping piece (which will resolve the other failing CI check you are seeing):

• Could you run changie new as described here so we can generate a changelog entry and credit you?

Speaking of CI, we'll want to:

• add a test similar to this so that we can catch any regressions in the future. Ideally, there would be a pre-existing test model to which a comment can be appended as a postfix.

[VersusFacit]

Hey @RobbertDM, thanks for your ping. I think Doug's right to make sure we have a test for this case. We have snapshot tests in tests/functional/simple_snapshot. Basically, add to our snapshot tests a new or existing file that describes the case where the model has your (pre-merge) failing use case. Should be as simple as writing a SQL definition and copying the format for a test class.

[RobbertDM]

Thanks for the guidance. I added a test file but I have little idea of what I am doing, so please review carefully :-)

[VersusFacit]

Awesome!! Thanks for the fast turnaround. It's basically all but there. You'll have to follow flake's directions of nixing unneeded names and such. It's perfectly fine you went with the conventions you saw. I think they pretty much look good for this case. We'll see with the adapter test results on your CI here.

[dbeatty10]

Thanks again for working on this PR @RobbertDM !

TL;DR

I think you'll ultimately need to add lines like the following:

    seeds__seed_csv,

and:

    @pytest.fixture(scope="class")
    def seeds(self):
        return {"seed.csv": seeds__seed_csv}

Do you want to give that a try?

Please let us know if you run into any problems and we'll try to help out.

More detail

I took a peek at the error during CI:

18:37:17  Completed with 1 error and 0 warnings:
18:37:17  
18:37:17  Database Error in snapshot snapshot_actual (snapshots/snapshot.sql)
18:37:17    relation "test16758814328907864877_test_comment_ending_snapshot.seed" does not exist
18:37:17    LINE 21:     select * from dbt.test16758814328907864877_test_comment_...

It's trying to snapshot a table named dbt.test16758814328907864877_test_comment_ending_snapshot.seed, but it's not finding that table. So I think the main thing to do is make sure it exists.

I didn't confirm, but I suspect that you need build the seed table with something like the following two pieces:

1. dbt-core/tests/functional/simple_snapshot/test_basic_snapshot.py

   Lines 9 to 10 in 0ef9931

   	seeds__seed_newcol_csv,
   	seeds__seed_csv,

2. dbt-core/tests/functional/simple_snapshot/test_basic_snapshot.py

   Lines 111 to 113 in 0ef9931

   	@pytest.fixture(scope="class")
   	def seeds(self):
   	return {"seed_newcol.csv": seeds__seed_newcol_csv, "seed.csv": seeds__seed_csv}

I'm assuming the seed_newcol portions in the examples above are not applicable to your case, so you can remove them (but also wouldn't hurt to leave them in initially just to make sure all the CI is working after copy-pasting).

[RobbertDM]

Hey @dbeatty10 thanks a lot for the detailed help, I wanted to give up after getting nothing useful from CI.
Where did you get that CI output?
I looked at this https://github.com/dbt-labs/dbt-core/actions/runs/4127200194 but that doesn't give me your output.
E: oh, there's also https://github.com/dbt-labs/dbt-core/actions/runs/4127200189

[dbeatty10]

Happy to help @RobbertDM !

I saw that error listed here.

It sounds like you looked at this top one:

And I scrolled down a bit and dug into one of these (which might all be basically identical):

[dbeatty10]

@RobbertDM

The CI isn't quite happy yet. I accidentally steered you wrong based on your test setup 😅 . If you update def seeds(self) to def seeds(), it might make the CI happy 🤞

[RobbertDM]

Yeah sorry, I wasn't thinking, just copy pasted :s

[iknox-fa]

@RobbertDM
I made a few small tweaks to your test case for you. You had a few issues:

• Trying to combine a class-based approach with a function-based approach, and pytest was having none of it.
• Not finding the seed table because you hadn't actually populated it by running the SQL file
• Needed to run snapshot to test your fix, not run, and crucially, you need to do it twice because the bug only happened when the snapshot table already exists in the DW.
• A bunch of unneeded fixtures. They didn't hurt anything, but they weren't being used.

Should be all set now. :)

(I also added a comment to the macro since we don't want someone coming along and cleaning up that whitespace you added at later date)

@VersusFacit
Mea Culpa for jumping in on an adapter ticket. I got tagged for review somehow and got halfway through before I realized I probably shouldn't have been. 🤓

Added code, requires another reviewer

[VersusFacit]

Huge thanks to everyone for keeping us alive and getting it across the finish line

[RobbertDM]

Woah, thanks a lot for all the help and explanation everyone. I'll definitely come back to this PR if I make another one.