-
Notifications
You must be signed in to change notification settings - Fork 358
feat!: linting rule no unregistered external models #5009
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,7 @@ | |
| "noselectstar", | ||
| "nomissingaudits", | ||
| "nomissingowner", | ||
| "nomissingexternalmodels", | ||
| ], | ||
| ), | ||
| ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import os | ||
|
|
||
| from sqlmesh import Context | ||
|
|
||
|
|
||
| def test_no_missing_external_models(tmp_path, copy_to_temp_path) -> None: | ||
| """ | ||
| Tests that the linter correctly identifies unregistered external model dependencies. | ||
|
|
||
| This test removes the `external_models.yaml` file from the sushi example project, | ||
| enables the linter, and verifies that the linter raises a violation for a model | ||
| that depends on unregistered external models. | ||
| """ | ||
| sushi_paths = copy_to_temp_path("examples/sushi") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we even need the sushi project here? I think we could get the same violation but simply creating an empty project with a model such as This way we don't have to:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am curious to understand the objection to this test. I don't particularly understand the rationale of the things you mention we avoid. While they are slightly inconvenient, the test is 50 lines, but because it's testing a fully fledged project, it gives me a lot of confidence. The only argument I can get is slow tests, but if we're optimising for that, we should be using other tools. Another way, I kind of read this is maybe about a philosophical difference in testing: The test you describe is the most isolated, unit-y test you can build, it's as minimal a test as possible to prove the thing works whereas mine feels like I start with the integration tests and test the function way more thoroughly because I exercise through a lot more scenarios. A minimal unit test wouldn't reveal, for example, any false positive, but running through sushi, which exercises a lot of variations, just might. Someone once described this testing to me as "happy path" testing. You build a robust happy e2e test (e.g. our sushi project) and then you exercise paths around it by altering the inputs.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not an outright objection but it also doesn't feel like the right tool for this. My reasoning being:
It makes sense to use the pre-existing projects such as |
||
| sushi_path = sushi_paths[0] | ||
|
|
||
| # Remove the external_models.yaml file | ||
| os.remove(sushi_path / "external_models.yaml") | ||
|
|
||
| # Override the config.py to turn on lint | ||
| with open(sushi_path / "config.py", "r") as f: | ||
| read_file = f.read() | ||
|
|
||
| before = """ linter=LinterConfig( | ||
| enabled=False, | ||
| rules=[ | ||
| "ambiguousorinvalidcolumn", | ||
| "invalidselectstarexpansion", | ||
| "noselectstar", | ||
| "nomissingaudits", | ||
| "nomissingowner", | ||
| "nomissingexternalmodels", | ||
| ], | ||
| ),""" | ||
| after = """linter=LinterConfig(enabled=True, rules=["nomissingexternalmodels"]),""" | ||
| read_file = read_file.replace(before, after) | ||
| assert after in read_file | ||
| with open(sushi_path / "config.py", "w") as f: | ||
| f.writelines(read_file) | ||
|
|
||
| # Load the context with the temporary sushi path | ||
| context = Context(paths=[sushi_path]) | ||
|
|
||
| # Lint the models | ||
| lints = context.lint_models(raise_on_error=False) | ||
| assert len(lints) == 1 | ||
| assert ( | ||
| "Model 'sushi.customers' depends on unregistered external models: " | ||
| in lints[0].violation_msg | ||
| ) | ||
Uh oh!
There was an error while loading. Please reload this page.