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

test(integv2): fixes to allow test_record_padding to partially run #5099

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

Conversation

johubertj
Copy link
Contributor

@johubertj johubertj commented Feb 7, 2025

Description of changes:

  • Adding debugging statements that were used to help solve the issue
  • Move the check for close marker from the end of all I/O operations to every individual iteration
  • Change the assert statement to assert for a non zero value and check for stderr

Problem

The way the I/O loop previously operated is shown w/ the below psuedo code.

for file_descriptor in [stdout, stderr]:
   if file_descriptor.is_empty():
       continue
    
   data_str = file_descriptor.read()

if close_marker in data_str:
   done

For a test to exit successfully, we must detect the close marker from stdout. Previously stderr was always empty so data_str was always populated from stdout.

In PR #3945, s2n-tls behavior was changed so that improper shutdowns cause errors. This affected test_record_padding.py because stderr was no longer empty. While the close marker was still read from stdout into data_str, the second iteration of the I/O loop overwrote the close marker with the message from stderr.

This change solves this problem by checking for the close marker every iteration.

Related Issue

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

@johubertj johubertj self-assigned this Feb 7, 2025
@github-actions github-actions bot added the s2n-core team label Feb 7, 2025
@johubertj johubertj marked this pull request as ready for review February 7, 2025 22:52
tests/integrationv2/processes.py Outdated Show resolved Hide resolved
tests/integrationv2/processes.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
@johubertj johubertj requested a review from jmayclin February 10, 2025 18:44
@johubertj johubertj requested a review from dougch as a code owner February 13, 2025 00:09
@johubertj johubertj closed this Feb 13, 2025
@johubertj johubertj force-pushed the feature-recordPaddingTestFix branch from 917d78b to 910665b Compare February 13, 2025 19:52
@johubertj johubertj reopened this Feb 13, 2025
@johubertj johubertj removed the request for review from dougch February 13, 2025 20:18
Copy link
Contributor

@jmayclin jmayclin left a comment

Choose a reason for hiding this comment

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

Now that it's passing, let's go ahead and addtest_record_padding back to the CI:

INTEGV2_TEST:
- "test_client_authentication test_dynamic_record_sizes test_sslyze test_sslv2_client_hello"
- "test_happy_path"
- "test_cross_compatibility"
- "test_early_data test_well_known_endpoints test_hello_retry_requests test_sni_match test_pq_handshake test_fragmentation test_key_update"
- "test_session_resumption test_renegotiate_apache test_buffered_send"
- "test_npn test_signature_algorithms"
- "test_version_negotiation test_external_psk test_ocsp test_renegotiate test_serialization"

@johubertj johubertj requested a review from jmayclin February 19, 2025 23:30
tests/integrationv2/processes.py Outdated Show resolved Hide resolved
@johubertj johubertj changed the title Fixes to have test_record_padding test pass test(integv2): fixes to allow test_record_padding to partially run Feb 20, 2025
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.

2 participants