Adding in NoMissingUnitTest linter rule #5294
Conversation
|
|
cmgoffena13
force-pushed
the
add-linter-rule
branch
from
8441fa1 to
81aed85
Compare
|
@VaggelisD @georgesittas - alright, took my first swing at it if either of you want to take a look.
I did surface Let me know if you want me to change anything! |
cmgoffena13
force-pushed
the
add-linter-rule
branch
from
2b486bc to
0c27875
Compare
sqlmesh/core/test/discovery.py
Outdated
| @property | ||
| def model_name(self) -> str: | ||
| return self.body["model"] |
There was a problem hiding this comment.
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. We don't wanna throw a KeyError in that case.
| @property | |
| def model_name(self) -> str: | |
| return self.body["model"] | |
| @property | |
| def model_name(self) -> str: | |
| return self.body.get("model", "") |
There was a problem hiding this comment.
Gotcha, quick fix, I'll get it done
sqlmesh/core/loader.py
Outdated
| model_test_metadata: t.List[ModelTestMetadata] | ||
| models_with_tests: t.Set[str] |
There was a problem hiding this comment.
Why is this an attribute of the LoadedProject? We just need the metadata right? Then you other consumers (e.g., the context, or linter) can extract this info and use it as needed.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Make sure this is computed once and is kept in the context so that we don't recompute it for every model. It is the same value for all models, anyway.
sqlmesh/core/context.py
Outdated
| raise SQLMeshError(f"Gateway '{gateway}' not found in the available engine adapters.") | ||
| return self.engine_adapter | ||
|
|
||
| def _filter_preloaded_tests( |
There was a problem hiding this comment.
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.
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.
Ah, makes sense. I suggest renaming this to _select_tests instead, then.
georgesittas
left a comment
georgesittas
left a comment
There was a problem hiding this comment.
Btw, let's make sure that we track calls to load_model_tests and refactor as needed. For example, in magics.py, I think we should be able to skip that call since we already have the loaded context which contains all tests. Another example is in lsp/context.py.
sqlmesh/core/context.py
Outdated
| 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]) |
There was a problem hiding this comment.
These iterations over test_meta look costly. Can we store a mapping test_name -> test_meta instead of keeping everything in a list and having to walk it every time?
There was a problem hiding this comment.
Just pushed, I added in two dictionaries, one for the fully qualified names and another for the Paths so they're just lookups. Calculated once in the context.
4 participants
I'm having trouble fleshing out the test due to the path having to point to the example project. Since I have to trigger the pytest from the root it is trying to find the /tests directory in the root rather than the example project root. Any help on resolving the path issue would be appreciated so I can finish the test. (EDIT: Figured out how to switch the contexts, let me know if there's anything that needs adjustment!)