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

Investigate why the BQ integration test didn't fail for quoted column name #540

Closed
joellabes opened this issue Apr 8, 2022 · 4 comments · Fixed by #569
Closed

Investigate why the BQ integration test didn't fail for quoted column name #540

joellabes opened this issue Apr 8, 2022 · 4 comments · Fixed by #569
Assignees
Labels
bug Something isn't working testing

Comments

@joellabes
Copy link
Contributor

joellabes commented Apr 8, 2022

Describe the bug

We caused #536 (fixed in #537) because of this change which was designed to avoid a similar issue in reverse from Snowflake (that it would have uppercased the column and failed to match in jinja, see #484.

Looking at the Circle logs for the job, it turns out that the table did error, but:

  • the subsequent dbt test invocation completed with exit code 0 (i.e. only the last step's outcome matters to Circle)
  • the dbt test invocation succeeded because even though the table failed to build, we don't generate unique schemata for each PR so it would have found an old copy of the table from a prior CI run.
  • Related: the fact that all PRs share a common schema also means that if two people push up commits at the same time, dbt will stop all over each other's tests and you get weird errors. This also happens if the same person commits back to back, but that's less of an issue IMO (I don't think we need a custom schema for every git commit for example).

Steps to reproduce

  • Cause a failure in an earlier stage of an invocation, that doesn't impact the final dbt test invocation (see here)

Expected results

CI to fail, because something didn't work

Actual results

No error because dbt test (the final step) still succeeds

Screenshots and log output

Linked above, but in case Circle has thrown away the logs by the time we get to this: utils-bq-logs.txt

@joellabes joellabes added bug Something isn't working triage labels Apr 8, 2022
@dbeatty10
Copy link
Contributor

dbeatty10 commented Apr 12, 2022

(The format of this response is an experiment utilizing a subset of the MADR template.)

Context and Problem Statement

Is there a way to cause run_test.sh to fail as soon as any of its commands fail? Or is there a better alternative to execute integration tests besides using run_test.sh?

Considered Options

  • set -e
  • command || exit 1
  • dbt build
  • move each step to CI
  • individual schemas per CI execution

Pros and Cons of the Options

set -e

This option is explained here.

  • Good, because adding a single line might solve our problems
  • Bad, because the reasons listed here

command || exit 1

This option is explained here.

  • Good, because it allows fine-grained control of which commands can fail the script
  • Good, because it avoids the potential issues of set -e
  • Bad, because harder to read for those of us not expert in bash

dbt build

This option would put all the integration test logic into a single command that would be the last of the script, and its exit code would be the exit code of the script overall.

  • Good, because the build sub-command is designed to do seed, run, and test all-in-one
  • Bad, because the test for the insert_by_period appears to rely on being executed stand-alone in --full-refresh mode

Move each step to CI

This option would move the logic from run_test.sh into the .circleci folder.

  • Good, because any integration step that fails will cause CI to fail immediately
  • Bad, because then developers won’t be able to the run the integration test script locally

Individual schemas per CI execution

Use a custom (randomized) schema name for each invocation of the integration test suite. Potentially drop the schema upon success and persist for X days afterwards to enable troubleshooting.

  • Good, because this scenario would have eventually failed (correctly) if there were unique schema names per invocation
  • Good, because multiple simultaneous CI executions will work
  • Bad, because the number of schemas (and relations) will proliferate if they are not cleared out on some regular cadence
  • Bad, because it would be resource-intensive to make the changes to enable unique schema names per invocation

@dbeatty10 dbeatty10 added testing and removed triage labels Apr 12, 2022
@jasnonaz
Copy link
Contributor

This is an awesome investigation into possible solutions here - thank you @dbeatty10!!!

Feels like if we could get this working off of dbt build alone that would be a tremendous win. Two things to think through there on how we might mitigate the downside of it not working with insert-by-period.

  • In the future of utils discussion, Joel wrote about moving the insert_by_period materialization into the experimental feature repo. If we were to go ahead and do that then we wouldn't have to worry about tests for insert by period.
  • You've probably already thought of this but would the insert-by-period tests work on a dbt build --full-refresh?

Those points aside, curious to hear which one you think would be best to move forward on.

@dbeatty10
Copy link
Contributor

I'm interested in pursuing command || exit 1 as our initial response and then queue dbt build, etc as follow-ups.

Rationale:

  • minimally invasive and straight-forward
    • only requires adding || exit 1 suffix to 5 lines of code
  • solves the immediate problem
    • already tested it out in my local environment
  • moving us in the right direction and also completely reversible
    • it doesn't prevent us from pursuing the other ideas like dbt build, etc as the next logical steps to take

@jasnonaz
Copy link
Contributor

Beautiful - let's do it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants