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

convert 062_defer_state_tests #6616

Merged
merged 5 commits into from
Jan 19, 2023
Merged

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented Jan 14, 2023

resolves #6613
resolves #6617

Backport to 1.4.latest after merging (see #6617 (comment))

Description

  • Converted to pytest framework
  • Replace direct file reads/writes with common test utilities (write_file, rm_file). I was a bit inconsistent in terms of what I defined in + imported from fixtures.py, and what I just kept as strings defined inline.
  • These are complex & intricate test sequences. I did some light refactoring in places where tests had been over-aggressively duplicated over time, but I kept most of them.
  • As part of that refactoring work, I found a bug in the implementation of --favor-state that's currently included in v1.4.0-rc1 (feature/favor-state-node #5859). I'll open a separate issue + PR to include that commit. We should backport this PR to 1.4.latest, and include the fix for v1.4.0-rc2.
  • I do not believe this test needs to be run on any other adapters; it's completely core functionality. As such, I added it to tests/functional rather than tests/adapter, and I'll open follow-up PRs in adapter repos to remove this test entirely.

Adapter PRs

Checklist

Copy link
Contributor

@McKnight-42 McKnight-42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Favor state isn't a config i've heard of so will look into that a bit more, but the tests themselves all make sense and I don't see any logical issues

@jtcohen6 jtcohen6 force-pushed the jerco/convert-defer-state-tests branch from 103c036 to 45e64d0 Compare January 19, 2023 08:41
@jtcohen6
Copy link
Contributor Author

I forgot I included a commit (fa9d2a0) that reverted the fix (083d310), just to prove that the converted test really does catch the bug + prove the fix!

Reverting the revert now...

@jtcohen6 jtcohen6 merged commit 07a004b into main Jan 19, 2023
@jtcohen6 jtcohen6 deleted the jerco/convert-defer-state-tests branch January 19, 2023 10:00
github-actions bot pushed a commit that referenced this pull request Jan 19, 2023
* Fix --favor-state flag

* Convert 062_defer_state_tests

* Revert "Fix --favor-state flag"

This reverts commit ccbdcba.

* Reformat

* Revert "Revert "Fix --favor-state flag""

This reverts commit fa9d2a0.

(cherry picked from commit 07a004b)
jtcohen6 added a commit that referenced this pull request Jan 19, 2023
* convert 062_defer_state_tests (#6616)

* Fix --favor-state flag

* Convert 062_defer_state_tests

* Revert "Fix --favor-state flag"

This reverts commit ccbdcba.

* Reformat

* Revert "Revert "Fix --favor-state flag""

This reverts commit fa9d2a0.

(cherry picked from commit 07a004b)

* Add changelog entry

Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.4.latest cla:yes Skip Changelog Skips GHA to check for changelog file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-1823] [Bug] [1.4.0rc1] --favor-state not working as intended [CT-1820] 062_defer_state_tests
2 participants