dbt-core icon indicating copy to clipboard operation
dbt-core copied to clipboard

[Bug] parse-time validation when unit testing incremental model

Open dbeatty10 opened this issue 1 year ago • 3 comments

Is this a new bug in dbt-core?

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

Current Behavior

Suppose a model includes the is_incremental() macro.

If a unit test is added for that model, then an override like the following is required or else an error will occur:

    overrides:
      macros:
        is_incremental: true

Acceptance Criteria

We should have a parse time error when:

  • unit testing an incremental model but not providing an override for is_incremental
  • unit testing an incremental model overriding is_incremental: true but NOT providing an input for this

Steps To Reproduce

models/my_model.sql

-- is_incremental: {{ is_incremental() }}
select
    1 as id

models/_unit_tests.yml

unit_tests:

  - name: demo_unit_test_overrides

    given: []

    model: my_model

    expect:
      format: csv
      rows: |
        id
        1
dbt run -s models/my_model.sql
dbt build -s models/my_model.sql

💥 See error log below.

In order to work, the following needs to be added within the unit test YAML above:

    overrides:
      macros:
        is_incremental: true

Relevant log output

21:41:03    Compilation Error in unit_test my_model__demo_unit_test_overrides (models/_unit_tests.yml)
  'None' has no attribute 'database'

Environment

- OS:
- Python:
- dbt: 1.8 beta

Which database adapter are you using with dbt?

No response

Additional Context

No response

dbeatty10 avatar Feb 16 '24 21:02 dbeatty10

Curious what @MichelleArk @jtcohen6 you think here...

I actually think it's a good thing that we require you to be explicit about whether this is a unit test for the incremental model in "incremental" mode or in "full refresh" mode. I'm not convinced we should set a default here (is that a spicy take?). However, if we don't set a default, we should:

  • update our docs on unit testing incremental models
  • update the error message returned to be explicit "For incremental models you must set is_incremental to true or false in your unit test overrides" (or something similar) - right now, the error message is unreadable

graciegoheen avatar Feb 20 '24 23:02 graciegoheen

This particular error is probably happening because this was not overwritten, and the 'real' is_incremental macro was attempting to access this.database: https://github.com/dbt-labs/dbt-adapters/blob/main/dbt/include/global_project/macros/materializations/models/incremental/is_incremental.sql#L7

I'm not convinced we should set a default here (is that a spicy take?).

I'm not convinced either. It feels like either setting (is_incremental: true or is_incremental: false) could be perceived as surprising behaviour and we'd get issue reports either way.

update the error message returned to be explicit "For incremental models you must set is_incremental to true or false in your unit test overrides" (or something similar)

I like the idea of parse-time validation here! 👍


MichelleArk avatar Feb 20 '24 23:02 MichelleArk

Sounds good! Updated the issue name and acceptance criteria accordingly

graciegoheen avatar Feb 21 '24 00:02 graciegoheen

From refinement:

  • Can determine whether the model being tested relies on the is_incremental macro based on its depends_on.macros (rather than checking for materialized=incremental, which is less precise)
  • can also validate that is_incremental is being overriden with a boolean value

MichelleArk avatar Mar 11 '24 16:03 MichelleArk