Skip to content

Add renegotiate apache test to nix/uv/pytest #5352

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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

dougch
Copy link
Contributor

@dougch dougch commented Jun 10, 2025

Release Summary:

Resolved issues:

partial for #5040

Description of changes:

Start apache2 with the uvinteg bash function, allowing this test to pass with nix+uv+pytest.

Call-outs:

This adds a cleanup routine for apache2 configs, so when doing local runs of the integration tests, the apache2 configs get regenerated. This is not needed in CI, as we're throwing away Docker containers after the test runs.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Local, CI.
How can you convince your reviewers that this PR is safe and effective?
Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?

Remember:

  • Any change to the library source code should at least include unit tests.
  • Any change to the core stuffer or blob methods should include CBMC proofs.
  • Any change to the CI or tests should:
    1. prove that the test succeeds for good input
    2. prove that the test fails for bad input (eg, a test for memory leaks fails when a memory leak is committed)

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

@github-actions github-actions bot added the s2n-core team label Jun 10, 2025
@dougch dougch added type/nix related to nix and removed s2n-core team labels Jun 10, 2025
@dougch dougch marked this pull request as ready for review June 11, 2025 21:52
@dougch dougch requested a review from lrstewart June 11, 2025 21:52
Copy link
Contributor

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

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

I don't think the PR name is accurate. You're not adding the renegotiate apache to nix, and you're not just adding it to uv, you're switching all the nix tests over to uv.

Comment on lines +114 to +115
export PYTEST_ARGS="--provider-version $S2N_LIBCRYPTO -x -n auto --reruns=2 --durations=10 -rpfs --cache-clear --ignore-glob=*test_dynamic_record_sizes* --ignore-glob=*test_session_resumption*"
uvinteg_snapshot
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure the two excluded tests are obvious enough :/ I'm worried someone looking at the buildspec will think we now have parity between nix and the job that still uses codebuild images, and remove the codebuild image job.

And what's the benefit of this PR? It looks like nix used to be able to run test_session_resumption, but can't anymore?

Comment on lines 84 to -82
function integ {(set -e
apache2_start
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems wrong for uvinteg to start apache, but integ not to anymore. If the integ target doesn't work anymore, shouldn't we just remove it / replace it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/nix related to nix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants