Skip to content

Conversation

@cmgoffena13
Copy link
Contributor

@cmgoffena13 cmgoffena13 commented Sep 4, 2025

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!)

@CLAassistant
Copy link

CLAassistant commented Sep 4, 2025

CLA assistant check
All committers have signed the CLA.

@cmgoffena13
Copy link
Contributor Author

@VaggelisD @georgesittas - alright, took my first swing at it if either of you want to take a look.

  1. Added in a property method to ModelTestMetadata to get model_name from the body easily
  2. Added model_test_metadata and models_with_tests to be added when a project loads so a Set of model names is only calculated once upon load.
  3. Added self._model_test_metadata and self._models_with_tests to the context and Context::load() brings it in from loaded projects.
  4. Made sure the Context::test() utilizes the eagerly loaded test metadata instead of lazily loading it again

I did surface models_with_tests as a property in the Context to access for the linter rule.

Let me know if you want me to change anything!

Copy link
Contributor

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@georgesittas
Copy link
Contributor

Thanks for addressing the comments @cmgoffena13. I'll take another look soon.

@cmgoffena13
Copy link
Contributor Author

@georgesittas - bumping this now that we're past the first round of holidays

Copy link
Contributor

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cmgoffena13, I'll get this in and see if I can slightly refactor by removing / compacting some of the new context attributes.

@georgesittas georgesittas merged commit 5c51f1a into TobikoData:main Dec 2, 2025
31 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants