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

tests: make integV2 locally runnable #5029

Merged
merged 14 commits into from
Jan 14, 2025

Conversation

jmayclin
Copy link
Contributor

Resolved issues:

Closes #4342
Part of #5027

Motivation

When tests can't be easily run locally, development becomes incredibly painful. Iteration is slow, and debugging is difficult.

IntegV2 is hard to run locally for two and a half reasons.

Reason 1: Python - "𝒿𝓊𝓈𝓉 𝓅𝓎𝓉𝒽𝑜𝓃 𝓉𝒽𝒾𝓃𝑔𝓈" It is relatively tricky to pull in the correct dependencies for a python project. Pip environment management, versioning, etc. tox also confuses me, but perhaps this is just me being smooth brained.

Reason 2: Executable - We require that all executables are available (on the path!) to run the IntegV2 tests. This means that an engineer has to install apache, gnutls, java, openssl, etc. Which is a lot.

Reason 2.5: We hard code specific CI assumptions. This includes things like paths, environment variables, etc.

Description of changes:

To solve the python things, we use uv. uv provides a cargo-like python experience. Dependency versions are defined in pyproject.toml. uv automatically pulls in these dependencies and sets up the correct virtual environment. No pip install shenanigans required. uv also makes it incredibly easy to run linters/formatters (not added in this PR)

To solve the executable issues, we add a new path configuration fixture. It currently only supports s2nd/s2nc and the JavaSSL socket client. It will gracefully skip a test if the executable is not available. Additionally, it does not requires s2nd/s2nc to be installed to the PATH.

Call-outs:

For a full description of the proposed work, see this.

Testing:

When running

uv run pytest --provider-version openssl_3_0 --best-effort -x -rpfs -n auto -k happy_path

without the java client compiled, I see the following report.

SKIPPED [2640] fixtures.py:65: <class 'providers.OpenSSL'> is not available
SKIPPED [2524] fixtures.py:65: <class 'providers.GnuTLS'> is not available
SKIPPED [740] fixtures.py:65: <class 'providers.JavaSSL'> is not available
SKIPPED [148] fixtures.py:65: <class 'providers.SSLv3Provider'> is not available
================ 2788 passed, 6052 skipped in 86.19s (0:01:26) =================

With the java client compiled, I see the following report.

SKIPPED [2640] fixtures.py:65: <class 'providers.OpenSSL'> is not available
SKIPPED [2524] fixtures.py:65: <class 'providers.GnuTLS'> is not available
SKIPPED [148] fixtures.py:65: <class 'providers.SSLv3Provider'> is not available
================ 3528 passed, 5312 skipped in 106.05s (0:01:46) ================

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jmayclin jmayclin requested review from goatgoose and dougch January 13, 2025 20:07
@github-actions github-actions bot added the s2n-core team label Jan 13, 2025
@@ -22,7 +22,7 @@
Protocols.TLS13
]

SSLYZE_SCANS_TO_TEST = {
SSLYZE_SCANS_TO_TEST = [
Copy link
Contributor Author

@jmayclin jmayclin Jan 13, 2025

Choose a reason for hiding this comment

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

In newer versions of python, set initialization is not stable. So each pytest worker process was iterating through SSLYZE_SCANS_TO_TEST in a different order, which means that the "test collection" list was not the same across processes. This causes a fatal error.

Converting the set to a list makes a stable iteration order.

* pep8 code
* remove .python-version
tests/integrationv2/fixtures.py Show resolved Hide resolved
tests/integrationv2/fixtures.py Outdated Show resolved Hide resolved
tests/integrationv2/fixtures.py Outdated Show resolved Hide resolved
tests/integrationv2/test_buffered_send.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dougch dougch left a comment

Choose a reason for hiding this comment

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

uv is a neat little utility- agree that it's an improvement over tox/pyenv/virtenv. This PR does a few other useful things that are neat: OpenSSL 3 provider and a sslyze update. FWIW, uv and nix aren't mutually exclusive, it works fine inside of a nix devshell, and it might make sense to think more about where the python package management lives

"--provider-version",
action="store",
dest="provider-version",
default=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't really optional though? Even if it's invalid, it's a required field - (Or the if "fips" check needs to be proper python)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not actually changing any of the provider version, it was just picked up by the formatter. surgically reverted.

tests/integrationv2/conftest.py Outdated Show resolved Hide resolved
@jmayclin jmayclin requested review from dougch and goatgoose January 14, 2025 02:35
why two empty lines? one seems like it would be neater.
tests/integrationv2/conftest.py Outdated Show resolved Hide resolved
tests/integrationv2/conftest.py Show resolved Hide resolved
tests/integrationv2/fixtures.py Show resolved Hide resolved
tests/integrationv2/fixtures.py Show resolved Hide resolved
@jmayclin jmayclin requested a review from goatgoose January 14, 2025 19:30
For some reason, the more default pytest.fail seems to be breaking
something with the openssl versioning? Although not documented why. :(
tests/integrationv2/fixtures.py Show resolved Hide resolved
tests/integrationv2/providers.py Fixed Show fixed Hide fixed
tests/integrationv2/providers.py Fixed Show fixed Hide fixed
tests/integrationv2/providers.py Fixed Show fixed Hide fixed
tests/integrationv2/providers.py Fixed Show fixed Hide fixed
@jmayclin jmayclin added this pull request to the merge queue Jan 14, 2025
Merged via the queue into aws:main with commit e79b1b9 Jan 14, 2025
42 checks passed
@jmayclin jmayclin deleted the integv2-minimal-runnable branch January 14, 2025 22:50
@jmayclin jmayclin mentioned this pull request Jan 15, 2025
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve local usability of integration tests
3 participants