-
Notifications
You must be signed in to change notification settings - Fork 315
Adding in NoMissingUnitTest linter rule #5294
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
base: main
Are you sure you want to change the base?
Changes from all commits
776a679
346ad72
2aabde8
04a82c2
c1d20dc
c09d2a8
795ac64
0c27875
f18f7e4
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 |
|---|---|---|
|
|
@@ -115,6 +115,7 @@ | |
| ModelTestMetadata, | ||
| generate_test, | ||
| run_tests, | ||
| filter_tests_by_patterns, | ||
| ) | ||
| from sqlmesh.core.user import User | ||
| from sqlmesh.utils import UniqueKeyDict, Verbosity | ||
|
|
@@ -146,8 +147,8 @@ | |
| from typing_extensions import Literal | ||
|
|
||
| from sqlmesh.core.engine_adapter._typing import ( | ||
| BigframeSession, | ||
| DF, | ||
| BigframeSession, | ||
| PySparkDataFrame, | ||
| PySparkSession, | ||
| SnowparkSession, | ||
|
|
@@ -398,6 +399,8 @@ def __init__( | |
| self._standalone_audits: UniqueKeyDict[str, StandaloneAudit] = UniqueKeyDict( | ||
| "standaloneaudits" | ||
| ) | ||
| self._models_with_tests: t.Set[str] = set() | ||
| self._model_test_metadata: t.List[ModelTestMetadata] = [] | ||
| self._macros: UniqueKeyDict[str, ExecutableOrMacro] = UniqueKeyDict("macros") | ||
| self._metrics: UniqueKeyDict[str, Metric] = UniqueKeyDict("metrics") | ||
| self._jinja_macros = JinjaMacroRegistry() | ||
|
|
@@ -636,6 +639,8 @@ def load(self, update_schemas: bool = True) -> GenericContext[C]: | |
| self._excluded_requirements.clear() | ||
| self._linters.clear() | ||
| self._environment_statements = [] | ||
| self._models_with_tests.clear() | ||
| self._model_test_metadata.clear() | ||
|
|
||
| for loader, project in zip(self._loaders, loaded_projects): | ||
| self._jinja_macros = self._jinja_macros.merge(project.jinja_macros) | ||
|
|
@@ -647,6 +652,8 @@ def load(self, update_schemas: bool = True) -> GenericContext[C]: | |
| self._requirements.update(project.requirements) | ||
| self._excluded_requirements.update(project.excluded_requirements) | ||
| self._environment_statements.extend(project.environment_statements) | ||
| self._models_with_tests.update(project.models_with_tests) | ||
| self._model_test_metadata.extend(project.model_test_metadata) | ||
|
|
||
| config = loader.config | ||
| self._linters[config.project] = Linter.from_rules( | ||
|
|
@@ -1049,6 +1056,11 @@ def standalone_audits(self) -> MappingProxyType[str, StandaloneAudit]: | |
| """Returns all registered standalone audits in this context.""" | ||
| return MappingProxyType(self._standalone_audits) | ||
|
|
||
| @property | ||
| def models_with_tests(self) -> t.Set[str]: | ||
| """Returns all models with tests in this context.""" | ||
| return self._models_with_tests | ||
|
|
||
| @property | ||
| def snapshots(self) -> t.Dict[str, Snapshot]: | ||
| """Generates and returns snapshots based on models registered in this context. | ||
|
|
@@ -2220,7 +2232,9 @@ def test( | |
|
|
||
| pd.set_option("display.max_columns", None) | ||
|
|
||
| test_meta = self.load_model_tests(tests=tests, patterns=match_patterns) | ||
| test_meta = self._filter_preloaded_tests( | ||
| test_meta=self._model_test_metadata, tests=tests, patterns=match_patterns | ||
| ) | ||
|
|
||
| result = run_tests( | ||
| model_test_metadata=test_meta, | ||
|
|
@@ -2782,6 +2796,33 @@ def _get_engine_adapter(self, gateway: t.Optional[str] = None) -> EngineAdapter: | |
| raise SQLMeshError(f"Gateway '{gateway}' not found in the available engine adapters.") | ||
| return self.engine_adapter | ||
|
|
||
| def _filter_preloaded_tests( | ||
| self, | ||
| test_meta: t.List[ModelTestMetadata], | ||
| tests: t.Optional[t.List[str]] = None, | ||
| patterns: t.Optional[t.List[str]] = None, | ||
| ) -> t.List[ModelTestMetadata]: | ||
| """Filter pre-loaded test metadata based on tests and patterns.""" | ||
|
|
||
| if tests: | ||
| filtered_tests = [] | ||
| for test in tests: | ||
| if "::" in test: | ||
| filename, test_name = test.split("::", maxsplit=1) | ||
| test_path = Path(filename) | ||
| filtered_tests.extend( | ||
| [t for t in test_meta if t.path == test_path and t.test_name == test_name] | ||
| ) | ||
| else: | ||
| test_path = Path(test) | ||
| filtered_tests.extend([t for t in test_meta if t.path == test_path]) | ||
|
Comment on lines
+2813
to
+2818
Contributor
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. These iterations over |
||
| test_meta = filtered_tests | ||
|
|
||
| if patterns: | ||
| test_meta = filter_tests_by_patterns(test_meta, patterns) | ||
|
|
||
| return test_meta | ||
|
|
||
| def _snapshots( | ||
| self, models_override: t.Optional[UniqueKeyDict[str, Model]] = None | ||
| ) -> t.Dict[str, Snapshot]: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,8 @@ class LoadedProject: | |
| excluded_requirements: t.Set[str] | ||
| environment_statements: t.List[EnvironmentStatements] | ||
| user_rules: RuleSet | ||
| model_test_metadata: t.List[ModelTestMetadata] | ||
| models_with_tests: t.Set[str] | ||
|
Comment on lines
+67
to
+68
Contributor
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. Why is this an attribute of the
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. Yeah, good catch, I can move this to the linter and keep the core code clean. Or actually, the context might be a little better, but I'm open to both
Contributor
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. Make sure this is computed once and is kept in the context so that we don't recompute it on every model being linted. It's a global property. |
||
|
|
||
|
|
||
| class CacheBase(abc.ABC): | ||
|
|
@@ -243,6 +245,12 @@ def load(self) -> LoadedProject: | |
|
|
||
| user_rules = self._load_linting_rules() | ||
|
|
||
| model_test_metadata = self.load_model_tests() | ||
|
|
||
| models_with_tests = { | ||
| model_test_metadata.model_name for model_test_metadata in model_test_metadata | ||
| } | ||
|
|
||
| project = LoadedProject( | ||
| macros=macros, | ||
| jinja_macros=jinja_macros, | ||
|
|
@@ -254,6 +262,8 @@ def load(self) -> LoadedProject: | |
| excluded_requirements=excluded_requirements, | ||
| environment_statements=environment_statements, | ||
| user_rules=user_rules, | ||
| model_test_metadata=model_test_metadata, | ||
| models_with_tests=models_with_tests, | ||
| ) | ||
| return project | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,6 +20,10 @@ class ModelTestMetadata(PydanticModel): | |||||||||||||
| def fully_qualified_test_name(self) -> str: | ||||||||||||||
| return f"{self.path}::{self.test_name}" | ||||||||||||||
|
|
||||||||||||||
| @property | ||||||||||||||
| def model_name(self) -> str: | ||||||||||||||
| return self.body["model"] | ||||||||||||||
|
Comment on lines
+23
to
+25
Contributor
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. We should be a bit more conservative here with the lookup. IIRC the body is validated later at runtime, when the tests are actually ran, by which time it'll be too late if the test is missing its name and we don't wanna throw a
Suggested change
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. Gotcha, quick fix, I'll get it done |
||||||||||||||
|
|
||||||||||||||
| def __hash__(self) -> int: | ||||||||||||||
| return self.fully_qualified_test_name.__hash__() | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
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.
What motivated this addition? Seems like it's something that doesn't need to be included in the scope of this PR.
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.
If I remember correctly, the test() method was setup to still lazily load the tests, so I had to implement this to ensure it maintained filtering functionality and utilized the eagerly loaded tests instead.
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.
Ah, makes sense. I suggest renaming this to
_select_testsinstead, then.