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

Fix tests inside of docker #573

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

Erotemic
Copy link
Contributor

I've run into a tricky issue when running tests locally inside of a docker environment. Tests that involved changing permissions to make a file unreadable were failing when I ran them inside of docker, but they worked on my host system. After a bit of thinking I realized this was because the default user in docker is root, which means files can be read even if they are unreadable according to the permissions.

To address this, I made two changes:

  • In test_core, I check if I can still read the file after it is forced to be unreadable. If it can be, then we can assume we are in some special circumstance, and we should skip the test.

  • In test_util, it looks like there is a custom path object, so I fell back to just checking if os.geteuid() == 0, which should only be true on linux for the root user. In this case I skip the test.

Because this is only a test change, I'm not sure if you want a changelog entry.

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (d539f42) to head (5403593).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #573   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         6466      6466           
=========================================
  Hits          6466      6466           
Flag Coverage Δ
py3.10 85.73% <ø> (ø)
py3.10_all 98.70% <ø> (ø)
py3.10_pydantic1 48.88% <ø> (ø)
py3.10_pydantic2 48.66% <ø> (ø)
py3.10_types 98.71% <ø> (ø)
py3.11 85.72% <ø> (ø)
py3.11_all 98.66% <ø> (ø)
py3.11_types 98.68% <ø> (ø)
py3.12 85.86% <ø> (ø)
py3.12_all 98.66% <ø> (ø)
py3.12_types 98.68% <ø> (ø)
py3.7 86.36% <ø> (ø)
py3.7_all 99.32% <ø> (ø)
py3.7_types 99.37% <ø> (ø)
py3.8 86.36% <ø> (ø)
py3.8_all 99.33% <ø> (ø)
py3.8_types 99.36% <ø> (ø)
py3.9 86.29% <ø> (ø)
py3.9_all 99.28% <ø> (ø)
py3.9_types 99.30% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mauvilsa
Copy link
Member

Running tests as root doesn't sound like a great idea. Better to use a real user so that the tests are run in an environment closer to how the code will be used later. And if you want to run the tests, skipping tests shouldn't be the go to solution.

I would recommend that you run the tests as a non-root user. In docker it is easy to switch user. If for some reason you really don't want to do that, from the command line you can tell pytest to skip some tests. No need to change the code.

pytest -k "not <test-name-1> and not <test-name-2> ..."

@Erotemic
Copy link
Contributor Author

Erotemic commented Sep 13, 2024

Yes, it is better to use a non-root user. Unfortunately, running as root in docker is the de-facto standard. Do I like it? No. But it is the reality.

I feel that just running pytest in the repo should "just work" in reasonable circumstances. But I also feel I'm going to have no success in arguing a point here. 😮‍💨

@mauvilsa
Copy link
Member

I have no problem in accepting this pull request. If you really want to go ahead a run tests as root, I can comment a couple of things and later we merge it.

But a clarification. Running tests as root is not a standard. The tests run here via circle ci all use docker images. They run as non-root and I didn't anything special for this. Maybe the github tests also run in docker containers, I am not sure, but if they do, then they run as non-root by default.

Base docker images do come as default as root. Though, the idea is that layers are added on top and the user changed. It is a good practice to never run docker containers as root.

jsonargparse_tests/test_util.py Outdated Show resolved Hide resolved
jsonargparse_tests/test_core.py Outdated Show resolved Hide resolved
jsonargparse_tests/test_core.py Outdated Show resolved Hide resolved
@Erotemic Erotemic force-pushed the investigate-test-failures branch 2 times, most recently from 4c1523d to 136674e Compare September 13, 2024 17:03
Copy link

sonarcloud bot commented Sep 13, 2024

@Erotemic
Copy link
Contributor Author

If you really want to go ahead a run tests as root, I can comment a couple of things and later we merge it.

Thanks. I wish running as non-root was an option, but I'm working in an environment where everything is running on a base python:3.11 image and there is little-to-no opportunity to change that. Others will install their packages to hard coded paths (e.g. /root/<theirstuff>) so if I swapped out the shared base image for something with a builtin non-root user then that would cause breakage. For better or worse I'm stuck in this environment, and this isn't the first time I've worked in a team where root-user docker images was the norm. For my part, I just want to be able to run through test suites (and show others how to run through the tests suites) with an invocation of pytest.

Having a root-user environment certainly isn't a problem for professional CI systems, but when working on teams with developers at many different skill levels, it can and does come up. I appreciate your willingness to help me out by adding code to detect and work around this admittedly non-ideal enviornment.

@mauvilsa mauvilsa merged commit ec7741a into omni-us:main Sep 16, 2024
29 checks passed
@mauvilsa mauvilsa added the refactor Improvements to the quality of the code label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Improvements to the quality of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants