Skip to content
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

Refactor for clarity that deployer/tests aren't just developer tests #971

Closed
consideRatio opened this issue Feb 2, 2022 · 5 comments · Fixed by #3094
Closed

Refactor for clarity that deployer/tests aren't just developer tests #971

consideRatio opened this issue Feb 2, 2022 · 5 comments · Fixed by #3094
Assignees

Comments

@consideRatio
Copy link
Member

consideRatio commented Feb 2, 2022

Description of problem and opportunity to address it

The deployer CLI rely on pytest as a production dependency - not just development dependency. This is because it use pytest and the tests in deployer/tests as a way to verify that hub's are functional.

From a glance, I would assume that those tests would be tests associated with testing the deployer CLI itself, because that is a pattern I've seen across the python ecosystem repeated. This is the first time I see pytest being used a real requirement for a project. Due to that, I suggest we are more explicit about this somehow.

Implementation guide and constraints

Should pytest be used at all?

@sgibson91 mentioned to me that @yuvipanda wants to avoid using pytest as a production dependency. That sounds reasonable to me and would solve this issue as well.

Pytest and logging

@sgibson91 also noted that there has been issues with a hub health-check test that fails could lead to pytest's verbosity exposes sensitive credentials. By doing a test manually, we would also avoid such complexity. This is related to:

Updates and ongoing work

  • 2023-10-11, this issue is still relevant but low prio.
@damianavila
Copy link
Contributor

So, we should rename those as integration tests? And if we are not going to use pytest for those tests, what do we use instead?

@sgibson91
Copy link
Member

And if we are not going to use pytest for those tests, what do we use instead?

I think we don't need to use a testing module at all for these, just call them as normal functions and feedback any errors from the notebooks

@choldgraf
Copy link
Member

choldgraf commented Feb 4, 2022

If we can make the assumption that notebooks like that "just need to run, top to bottom, without any errors" then it seems the simplest to just use something like nbclient to make sure that this is the case.

There are also some notebook testing tools out there (for example, testbook from nteract) but that might be more complexity than we need. Not sure if this is relevant, but nbclient recently added hooks as an option during execution, maybe that would be useful if we wanted to check things throughout the execution process?

@damianavila
Copy link
Contributor

Yep, nbclient seems a reasonable choice.

@pnasrat
Copy link
Contributor

pnasrat commented Feb 16, 2023

I was also confused by this layout and the simplification to pure healthchecking functions outside of pytest makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
6 participants