feat(test): treat model file paths as test selectors - #6061
Open
tripleaceme wants to merge 1 commit into
Open
Conversation
`sqlmesh test` already accepted multiple positional arguments, but they were only ever looked up in the test-file index. Passing a model file matched nothing and exited 0, so a commit hook handing over staged model files silently ran no tests at all. Each positional argument now resolves to a test file, a `file.yaml::test_name`, or a model file, in which case the tests targeting that model are selected. The results are unioned and deduplicated on the fully qualified test name, so a model file and the test file holding its tests select the same test once rather than running it twice. An argument that resolves to neither a known test nor a known model file is now an error instead of a silent skip. That is opt-in via raise_on_unknown_paths and only the CLI turns it on, because LSPContext.get_document_tests selects against arbitrary open documents and relies on an empty result. A known model that simply has no tests still selects nothing, which is not an error. Selectors are matched as given before being normalized, so relative paths work from the project root without any resolve() calls per model. The model path index is built at most once per call, and only when a selector is not a test file. Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes #6023.
sqlmesh testalready took multiple positional arguments, but they were only ever looked up in the test-file index. A model file matched nothing and the command exited 0, so a pre-commit hook passing staged model files silently ran no tests.Each positional argument now resolves to one of:
file.yaml::test_name→ that one testThe results are unioned. Per your note on the issue, they are also deduplicated on
fully_qualified_test_name, so overlapping selectors don't cause duplicate runs —sqlmesh test models/a.sql tests/test_a.yaml, where the YAML holds a's tests, runs each of them once, andtests/a.yaml::test_x tests/a.yamlcollapses to one run too.An argument resolving to neither a known test nor a known model file is now an error rather than a silent skip. That part is opt-in through
raise_on_unknown_pathsand only the CLI turns it on, becauseLSPContext.get_document_testscallsselect_testswith whatever document happens to be open and depends on getting an empty list back — raising unconditionally would make the extension throw on every non-test file. A known model that simply has no tests still selects nothing and is not an error, since failing a hook because a model lacks a unit test isn't the intent here.--select-modelis unchanged: it still narrows the selection rather than adding to it.Two notes on cost, since path matching is on the hot path for the pre-commit use case:
os.path.abspathrather thanresolve()— no stat or symlink syscalls per model.Test Plan
Nine tests in
tests/core/test_test.py:test_model_path_selects_its_tests/test_model_path_without_tests_selects_nothingtest_model_and_test_paths_are_unioned_without_duplicates— the dedup case from your commenttest_overlapping_yaml_and_named_test_are_deduplicatedtest_relative_paths_select_tests— pre-commit passes repo-relative pathstest_unknown_path_is_ignored_by_default— guards the LSP's behaviortest_unknown_path_errors_when_requested/test_unknown_test_name_errors_when_requestedtest_select_model_still_filters_path_selectionTwo in
tests/cli/test_cli.pycovering the CLI wiring and the non-zero exit on an unknown path.The 4 remaining failures in those files (
test_pyspark_python_model, threetest_dlt_*) are pre-existing onmainin my environment — missingpysparkanddlt.Checklist
make styleand fixed any issuesmake fast-test)git commit -s) per the DCO