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

Running twine tests in dev environment fail and emit PyPI tokens #1121

Closed
1 task done
jaraco opened this issue Jun 24, 2024 · 8 comments · Fixed by #1153 · May be fixed by #1131
Closed
1 task done

Running twine tests in dev environment fail and emit PyPI tokens #1121

jaraco opened this issue Jun 24, 2024 · 8 comments · Fixed by #1153 · May be fixed by #1131
Assignees
Labels
bug testing Test frameworks, tests, etc.

Comments

@jaraco
Copy link
Member

jaraco commented Jun 24, 2024

Is there an existing issue for this?

  • I have searched the existing issues (open and closed), and could not find an existing issue

What keywords did you use to search existing issues?

tests isolation

Please describe why your using this option

Running tox in a clean checkout of twine produces 5 failures:

FAILED tests/test_package.py::test_pkginfo_returns_no_metadata[unsupported Metadata-Version] - Failed: DID NOT RAISE <class 'twine.exceptions.InvalidDistribution'>
FAILED tests/test_settings.py::test_password_is_required_if_no_client_cert[None] - AssertionError: assert 'pypi-AgEIcHl...<elided>' == 'entered pw'
FAILED tests/test_settings.py::test_password_is_required_if_no_client_cert[] - AssertionError: assert 'pypi-AgEIcHl...<elided> == 'entered pw'
FAILED tests/test_settings.py::test_password_required_if_no_client_cert_and_non_interactive - Failed: DID NOT RAISE <class 'twine.exceptions.NonInteractive'>
FAILED tests/test_settings.py::test_no_password_prompt_if_client_cert_and_non_interactive - AssertionError: assert not 'pypi-AgEIcHl<elided>...

It appears as if the presence of credentials in my keyring is causing tests to fail (and emit my credentials).

Anything else you'd like to mention?

No response

@jaraco
Copy link
Member Author

jaraco commented Jun 24, 2024

Running the tests on 4.0.2 produces a mostly different set of failures, seemingly related to build backend issues. Beginning with 5.0.0, the current set of failures is observed.

@jaraco jaraco added bug testing Test frameworks, tests, etc. and removed enhancement feature request labels Jun 24, 2024
@sigmavirus24
Copy link
Member

sigmavirus24 commented Jun 25, 2024

Integration tests broke nearly as soon as you added them over a year ago, but I knew you'd complain if I removed them and you didn't notice any of the pings. Brian ignored them before me and since they very occasionally pass, I've left them.

Master is failing now because Tres released an API breaking change in a minor version of pkginfo. I just haven't had time to fix it.

Edit Just realized this was a different issue you opened from the one about master tests being broken.

@jaraco
Copy link
Member Author

jaraco commented Jun 25, 2024

Master is failing now because Tres released an API breaking change in a minor version of pkginfo. I just haven't had time to fix it.

There appear to be two failure modes here. The first is in test_pkginfo_returns_no_metadata, which also fails in an environment without my credentials. That's probably the issue with pkginfo. It was reported in #1116. Let's disregard that one for the sake of this issue, which is about the other failures which happen in my dev environment due to the presence of my PyPI credentials.

@jaraco
Copy link
Member Author

jaraco commented Jun 25, 2024

It looks like when 2afda47 was introduced (#1040), it supersedes the username="fakeuser" in the tests, allowing keyring to resolve the user's PyPI token instead of the entered_password.

@jaraco
Copy link
Member Author

jaraco commented Jun 25, 2024

I'm unsure the best way forward here. An easy fix might be to disable keyring in tests - mock it out so it always behaves as if there's no password. Another option could be to make __token__ the default username when accessing PyPI or TestPyPI, but have it honor the specified username if supplied. @woodruffw do you have an opinion on the matter?

@woodruffw
Copy link
Member

Another option could be to make __token__ the default username when accessing PyPI or TestPyPI, but have it honor the specified username if supplied. @woodruffw do you have an opinion on the matter?

No strong opinion on the mocking idea, but I think this is likely to cause user confusion (since PyPI and TestPyPI will no longer accept a username at all for package upload, and will always fail hard.)

@jaraco jaraco self-assigned this Jun 25, 2024
jaraco added a commit that referenced this issue Jun 25, 2024
@jaraco
Copy link
Member Author

jaraco commented Jun 25, 2024

@sigmavirus24 or @bhrutledge : Do you have an opinion on how to deal with this scenario?

In ddd4ecb, I've drafted "token" as default if unspecified. It addresses the issue, but creates four new test failures (exactly the concern woodruffw raised):

FAILED tests/test_register.py::test_values_from_env_pypi[pypi] - AssertionError: assert '__token__' == 'this-is-ignored'
FAILED tests/test_register.py::test_values_from_env_pypi[testpypi] - AssertionError: assert '__token__' == 'this-is-ignored'
FAILED tests/test_upload.py::test_values_from_env_pypi[pypi] - AssertionError: assert '__token__' == 'this-is-ignored'
FAILED tests/test_upload.py::test_values_from_env_pypi[testpypi] - AssertionError: assert '__token__' == 'this-is-ignored'

It would be a breaking change to drop that expectation, but it feels like the cleanest approach. It also means that users have more control over their environment, but it means that those reliant on the current hard-coded behavior will need to remove any configured username to retain compatibility. Honestly, that feels best to me because the current approach is masking an unused configuration. It feels wrong for a user (or test) to be explicitly supplying a configuration but it's silently suppressed.

I'm also trying to avoid test-only behaviors. I'd like for the tests to exercise the system as closely as possible to how it operates in production, but since it's not possible to override the username or otherwise select a non-default username, the tests are hitting real credentials. It feels like the current design, overriding with a username="fakeuser" is the most genuine way to avoid hitting real credentials.

jaraco added a commit that referenced this issue Jun 25, 2024
@jaraco
Copy link
Member Author

jaraco commented Jun 25, 2024

In 2c4e4a1, I've committed the changes that allow the tests to pass. Interestingly, not all of 4a2dfb0 had to be reverted. The fixtures in test_settings still retain the behavior that __token__ takes precedence (because the default value takes precedence over config), so only users setting TWINE_USERNAME or specifying the username on the command line will see a change in behavior.

@jaraco jaraco changed the title Running twine tests in dev environment leads to workflow failures Running twine tests in dev environment fail and emit PyPI tokens Jun 26, 2024
jaraco added a commit that referenced this issue Jun 26, 2024
jaraco added a commit that referenced this issue Jun 26, 2024
stefanor added a commit to stefanor/twine that referenced this issue Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug testing Test frameworks, tests, etc.
Projects
None yet
3 participants