Skip to content

Conversation

@rcmadhankumar
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Mar 26, 2025

Codecov Report

Attention: Patch coverage is 81.57895% with 7 lines in your changes missing coverage. Please review.

Project coverage is 94.39%. Comparing base (4a7314e) to head (c2c8822).

Files with missing lines Patch % Lines
pytest_container/container.py 73.33% 4 Missing ⚠️
pytest_container/helpers.py 72.72% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #239      +/-   ##
==========================================
- Coverage   96.73%   94.39%   -2.34%     
==========================================
  Files           9        9              
  Lines        1101     1124      +23     
  Branches      164      159       -5     
==========================================
- Hits         1065     1061       -4     
- Misses         23       41      +18     
- Partials       13       22       +9     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rcmadhankumar
Copy link
Contributor Author

Working on the coverage.



def run_command(
cmd: list[str],
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't work in python < 3.9 (plus import it at the top if not present)

Suggested change
cmd: list[str],
cmd: List[str],

Comment on lines 4 to 6
`pytest_container` works with Python 3.6 and later and optionally requires `pytest
<https://pytest.org/>`_ and `pytest-testinfra
<https://testinfra.readthedocs.io/>`_. Additionally, for python 3.6, you'll need
Copy link
Owner

Choose a reason for hiding this comment

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

pytest is not optional, testinfra is

@rcmadhankumar
Copy link
Contributor Author

I saw this comment recently: #231 (comment)

dcermak Oct 19, 2024

I don't think that it is maintainable to start implementing a subset of testinfra in pytest_container. I think all that we should offer is check_output. If a user doesn't wish to use testinfra, then that's fine by me, but then they should handle these checks by themself.

I think I overworked this PR.

I touched the existing test cases to remove the host object from test infra and use run command from helpers.
But, i think now that all we need to do is

  • just enable the user to not use test infra if they want to
  • add unit tests for containerRemoteEndPoint module
  • Existing test cases should never be changed
  • Only need to make sure that files in pytest_container module is not using host object from testinfra
  • It is user's responsibility about how they use containerRemoteEndPoint, we'll do only unit tests for this module

@dcermak , please confirm and add if any additional stuff is needed.

@dcermak
Copy link
Owner

dcermak commented Mar 27, 2025

I saw this comment recently: #231 (comment)

dcermak Oct 19, 2024
I don't think that it is maintainable to start implementing a subset of testinfra in pytest_container. I think all that we should offer is check_output. If a user doesn't wish to use testinfra, then that's fine by me, but then they should handle these checks by themself.

I think I overworked this PR.

A bit:
image

I touched the existing test cases to remove the host object from test infra and use run command from helpers. But, i think now that all we need to do is

  • just enable the user to not use test infra if they want to
  • add unit tests for containerRemoteEndPoint module
  • Existing test cases should never be changed
  • Only need to make sure that files in pytest_container module is not using host object from testinfra
  • It is user's responsibility about how they use containerRemoteEndPoint, we'll do only unit tests for this module

@dcermak , please confirm and add if any additional stuff is needed.

I would that you start in a separate PR with the removal of the host from pytest_container

@rcmadhankumar rcmadhankumar force-pushed the testinfra-optional branch 2 times, most recently from c4d248b to 28f5034 Compare April 4, 2025 07:01
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.

2 participants