Skip to content

refactor(tests): data/ tests to follow test conventions#3247

Open
agsaru wants to merge 6 commits into
Netflix:masterfrom
agsaru:refactor-pytest-data
Open

refactor(tests): data/ tests to follow test conventions#3247
agsaru wants to merge 6 commits into
Netflix:masterfrom
agsaru:refactor-pytest-data

Conversation

@agsaru

@agsaru agsaru commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

PR Type

  • Bug fix
  • New feature
  • Core Runtime change (higher bar -- see CONTRIBUTING.md)
  • Docs / tooling
  • Refactoring

Summary

Copilot AI review requested due to automatic review settings June 8, 2026 19:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR refactors and expands S3-related tests to improve readability, determinism, and coverage around long/Unicode path handling, while also tightening some test utilities/fixtures.

Changes:

  • Refactored generate_local_path tests to use pytest.mark.parametrize with explicit case IDs and added a short-URL non-truncation test.
  • Cleaned up test_s3.py fixtures and parameterizations (adds IDs, moves fixtures, tightens exception handling).
  • Simplified and clarified reset_current_env teardown logic in conftest.py.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
test/data/s3/test_s3op.py Reworks local-path tests into parametrized cases; adds indicator coverage for long vs short URLs; removes redundant try/finally wrapper.
test/data/s3/test_s3.py Introduces/moves fixtures, replaces bare except, adds param IDs, and changes some S3 root usage in benchmark helpers.
test/data/s3/conftest.py Refactors current reset fixture teardown to restore original private attributes and remove newly created ones.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/data/s3/test_s3.py Outdated

def _do():
with S3(s3root=s3root) as s3:
with S3(s3root=S3ROOT) as s3:
Comment thread test/data/s3/test_s3.py Outdated
def _do():
new_files = [(str(uuid4()), path) for _, path in all_files]
with S3(s3root=s3root, inject_failure_rate=inject_failure_rate) as s3:
with S3(s3root=S3ROOT, inject_failure_rate=inject_failure_rate) as s3:
Comment thread test/data/s3/test_s3.py
@greptile-apps

greptile-apps Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR refactors the test/data/s3 test suite to follow pytest conventions: fixtures are moved to the top of the file, inline parametrize values are extracted into named constants, and loop-based tests are converted to parametrized form.

  • conftest.py: Fixes the previously reported ids length mismatch by making S3ROOT_IDS conditional on whether S3ROOT is set; the teardown logic correctly handles attribute restoration via its two-pass approach (Pass 1 removes new attrs, Pass 2 restores all saved attrs including any deleted during the test).
  • test_s3.py: Introduces INJECT_FAILURE_RATES/INJECT_FAILURE_RATE_IDS constants used across ~10 @pytest.mark.parametrize calls; adds the s3root fixture to test_put_one_benchmark and test_put_many_benchmark so they participate in the trailing-slash matrix, and removes two unused imports.
  • test_s3op.py / s3_data.py: Converts a loop-based test to parametrized form, splits a dual-assertion test into two focused tests, removes a no-op try/finally, and guards ensure_test_data() against S3ROOT=None.

Confidence Score: 5/5

Safe to merge — all changes are confined to the test suite, no production code is touched, and the two previously flagged issues are correctly resolved.

Every change is a structural refactoring of test files: extracting constants, reorganising fixtures, and converting loop-based tests to parametrized form. The previously reported ids mismatch is fixed, and the put benchmark functions now correctly use the s3root fixture. No new logic is introduced in production code paths.

No files require special attention.

Important Files Changed

Filename Overview
test/data/s3/conftest.py Fixes mismatched ids length by making S3ROOT_IDS conditional on S3ROOT; teardown correctly restores deleted attributes via Pass 2 iteration over saved_state
test/data/s3/test_s3.py Extracts INJECT_FAILURE_RATES/INJECT_FAILURE_RATE_IDS constants, moves fixtures to top of file, adds s3root fixture to put benchmarks so they participate in trailing-slash parametrization
test/data/s3/test_s3op.py Converts loop-based test_generate_local_path_length_limits to parametrized form; splits truncation test in two; removes no-op try/finally block
test/data/s3/s3_data.py Adds early-return guard in ensure_test_data() when S3ROOT is None, preventing a crash in environments without S3

Reviews (3): Last reviewed commit: "fixed conftest.py" | Re-trigger Greptile

Comment thread test/data/s3/conftest.py
Comment thread test/data/s3/test_s3.py Outdated
@agsaru agsaru marked this pull request as draft June 8, 2026 19:47
@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (master@732ca76). Learn more about missing BASE report.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3247   +/-   ##
=========================================
  Coverage          ?   28.82%           
=========================================
  Files             ?      381           
  Lines             ?    52467           
  Branches          ?     9260           
=========================================
  Hits              ?    15126           
  Misses            ?    36329           
  Partials          ?     1012           

☔ View full report in Codecov by Harness.
📢 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@agsaru agsaru marked this pull request as ready for review June 9, 2026 14:59
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