-
Notifications
You must be signed in to change notification settings - Fork 94
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
Fix missing snapshots #1456
Fix missing snapshots #1456
Conversation
d8781dc
to
2dbc0ad
Compare
|
||
# Run snapshots changes for metricflow-semantics | ||
# these are not dialect specific, so only need to run once | ||
run_command(f"pytest -x -vv -n 4 --overwrite-snapshots {MF_SEMANTICS_TEST_DIRECTORY}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put this to be ran under the duckdb
engine only, as we only need to run these updates once since it's not dialect specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test this to make sure it works as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup!
|
||
# Run snapshots changes for metricflow-semantics | ||
# these are not dialect specific, so only need to run once | ||
run_command(f"pytest -x -vv -n 4 --overwrite-snapshots {MF_SEMANTICS_TEST_DIRECTORY}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test this to make sure it works as expected?
Context
After merging #1452, seems like snapshots in MF-semantics were missing (CI still passed - odd). But updated those and also updated the script to update snapshots in MF semantics as that seems to be missing.