add user home dir #80
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #80 +/- ##
==========================================
+ Coverage 57.14% 62.36% +5.22%
==========================================
Files 5 5
Lines 154 186 +32
==========================================
+ Hits 88 116 +28
- Misses 66 70 +4 ☔ View full report in Codecov by Sentry. |
| global_home = Path(os.environ['JUPYTERHUB_USER_HOME']) | ||
| user_dir = global_home / username | ||
|
|
||
| # Ensure the system user exists | ||
| self._ensure_system_user(username, group='jupyterhub') | ||
|
|
||
| # Ensure the user directory exists and has correct permissions | ||
| self._ensure_user_directory(user_dir, username) | ||
|
|
||
| # Ensure the user's Jupiter directory exists | ||
| self._ensure_user_jupyter_directory(user_dir) | ||
|
|
There was a problem hiding this comment.
Why not move all of this into a helper method, and then you can test that?
There was a problem hiding this comment.
hmmm I don't see any benefit for that.
There was a problem hiding this comment.
You almost test the entirety of the start method, which is currently untested
There was a problem hiding this comment.
I think it's probably fine. Unless you'd prefer making every function a public method for testing. My goal is to keep it as modular as possible, so that if we need to make changes in the future (which seems likely), it will be easier to adapt.
There was a problem hiding this comment.
Unless you'd prefer making every function a public method for testing.
Nope, def not
My goal is to keep it as modular as possible, so that if we need to make changes in the future (which seems likely), it will be easier to adapt.
That's fine, but I don't see how calling _start_internal from start, having that wrap everything but stuff that touches self and testing _start_internal makes it less modular
There was a problem hiding this comment.
I'm fine with you not testing it for now, but I wouldn't be comfortable saying we wouldn't test it when productionizing the code, which is the message I was hearing here
There was a problem hiding this comment.
👍 I got your point. I’m actually fine with not dedicating excessive time to unit testing at this stage or even post production. I’d prefer to focus on making progress.
There was a problem hiding this comment.
I’m actually fine with not dedicating excessive time to unit testing at this stage
That's fine with me. If you write tests I review them, otherwise I don't. I think the only rule needs to be that we add test files that import new code so it gets marked in Codecov as uncovered.
or even post production
If I'm understanding what you mean here I don't think this is ok. The CDM is going to be a central, critical piece of infrastructure (unlike collections, which is off-to-the-side when it comes to KBase infrastructure) and is going to need automated testing with good coverage.
Also, to be frank I found it harder and harder to work on collections due to the lack of automated testing and possibility of regressions, which is why I'm adding more tests to the cdm task service than I did to collections
There was a problem hiding this comment.
I am confused. You are okay with just an import file but also insist on putting everything into a helper function and test it? It's either no test or making it test everything?
There was a problem hiding this comment.
I'm ok with just imports now, but before we go to production it needs coverage via automated tests
Co-authored-by: MrCreosote <[email protected]>
Co-authored-by: MrCreosote <[email protected]>
No description provided.