Skip to content

Fix Windows test failures and remove debug prints for DRF#9808

Merged
browniebroke merged 5 commits intoencode:mainfrom
Shrikantgiri25:fix-windows-tests-and-cleanup
Dec 5, 2025
Merged

Fix Windows test failures and remove debug prints for DRF#9808
browniebroke merged 5 commits intoencode:mainfrom
Shrikantgiri25:fix-windows-tests-and-cleanup

Conversation

@Shrikantgiri25
Copy link
Copy Markdown
Contributor

@Shrikantgiri25 Shrikantgiri25 commented Oct 24, 2025

What this PR does

  • Skips Windows-incompatible path test in TestRegularFieldMappings.test_regular_fields
  • Removes leftover print() statements in tests/schemas/test_openapi.py

Background / Why

While setting up DRF on Windows 11 with Python 3.13, I noticed:

  • TestRegularFieldMappings.test_regular_fields fails on Windows due to backslash escaping differences in path representations (repr(path) shows 'C:\\Users' on Windows)
  • Debug print() statements cluttered test output

How I fixed it

  • Added @pytest.mark.skipif(sys.platform.startswith("win"), reason="Test not supported on Windows") to skip the Windows-incompatible test (follows existing pattern used in test_decorators.py)
  • Removed all print() statements in test_openapi.py

Impact

  • Cleaner test output without debug prints
  • Windows users can run the test suite without failures (skipping the incompatible test)
  • Follows existing codebase patterns for platform-specific test skipping

Discussion / Reference

Original discussion: DRF Discussion #9807

@auvipy auvipy requested review from auvipy and Copilot October 25, 2025 07:38
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes cross-platform test failures on Windows with Python 3.13 and removes debug print statements from the test suite. The main changes ensure that path-based tests handle Windows backslashes correctly and that regex patterns are flexible enough to accommodate platform-specific variations in object representations.

  • Fixed Windows path escaping issues in TestRegularFieldMappings.test_regular_fields
  • Relaxed regex patterns to handle cross-platform differences in object string representations
  • Removed 7 debug print() statements from OpenAPI schema tests

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tests/test_model_serializer.py Updated regex patterns to handle Windows paths and cross-platform object representations; added re.DOTALL flag for multi-line matching
tests/schemas/test_openapi.py Removed debug print() statements that were cluttering test output

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

Comment thread tests/test_model_serializer.py Outdated
auvipy
auvipy previously approved these changes Oct 25, 2025
@p-r-a-v-i-n
Copy link
Copy Markdown
Contributor

I just wanted to share my perspective here:

  1. This change only affects a test and doesn’t impact any runtime behavior , so it adds more complexity compare to benefits.
  2. It might increases the chance of breaking the test if the serializer fields or repr() format change in future
  3. DRF generally aims to keep its tests simple and platform-agnostic. In practice, the test suite is not officially guaranteed to pass on native Windows.
  4. maintenance burden for non-production code.

windows user can use wsl

@Shrikantgiri25
Copy link
Copy Markdown
Contributor Author

Hi 👋 just following up on this PR.
It fixes a cross-platform test failure on Windows (Python 3.13) and improves setup experience for new contributors.
If no changes are needed, great — and if it’s not aligned with project scope, happy to close it. Thanks!

@browniebroke
Copy link
Copy Markdown
Member

The removal of the print statements make complete sense to me, but I'm a bit more on the fence on the rest of the PR. To Pravin's point, it does introduce some complexity which I'm not sure is worth it. It seems to also change some lines which are unrelated at first glance... I might be missing something here.

It's better to keep changes minimal and separated, if you send a PR for removing the print statement, I'm happy to accept it.

@Shrikantgiri25
Copy link
Copy Markdown
Contributor Author

The removal of the print statements make complete sense to me, but I'm a bit more on the fence on the rest of the PR. To Pravin's point, it does introduce some complexity which I'm not sure is worth it. It seems to also change some lines which are unrelated at first glance... I might be missing something here.

It's better to keep changes minimal and separated, if you send a PR for removing the print statement, I'm happy to accept it.

Hi @browniebroke ,
I noticed some tests are already skipped for version-specific reasons (e.g., in test_decorators.py). For this Windows case, instead of modifying the test logic, would it be acceptable to use a similar skip approach:

@pytest.mark.skipif(sys.platform.startswith("win"), reason="Skipped on Windows")

This would avoid introducing additional complexity.
If this isn’t desirable, I’m happy to submit a separate PR just for removing the debug print statements and close this PR.

Thanks for your guidance!

@browniebroke
Copy link
Copy Markdown
Member

browniebroke commented Nov 24, 2025

Yes that seems lower lighter touch fix yes 👍🏻

Shrikant Giri added 3 commits November 24, 2025 23:39
Windows requires manual backslash replacement for repr() matching:
- repr() displays C:\Users as 'C:\Users' (double backslashes)
- Regex needs \\ to match \ (four backslashes in pattern)
- re.escape() only produces \ (matches single backslash)

Test evidence on Windows:
  re.escape(path) in repr(path): None (fails)
  path.replace('\', r'\\') in repr(path): Match (works)

Solution:
- Windows: Manual replacement for repr() double-escaping
- Unix: re.escape() for special character handling

Addresses @auvipy's review with detailed testing and explanation.
@Shrikantgiri25
Copy link
Copy Markdown
Contributor Author

Yes that seems lower touch fix yes 👍🏻

Great, thanks! I'll update the PR to use the skip approach + print removals.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Shrikantgiri25 Shrikantgiri25 force-pushed the fix-windows-tests-and-cleanup branch from 43da74e to c601602 Compare November 24, 2025 18:34
@Shrikantgiri25
Copy link
Copy Markdown
Contributor Author

Hi all, updated the PR to use @pytest.mark.skipif for the test.

@Shrikantgiri25
Copy link
Copy Markdown
Contributor Author

Hi @auvipy

I hope you’re doing well. I wanted to kindly follow up on this PR. I’ve updated it based on the initial feedback and would be happy to make any further adjustments if needed.
I completely understand you might be busy and really appreciate your time and guidance whenever you get a chance to review.

Thank you!

@browniebroke browniebroke merged commit e027f0c into encode:main Dec 5, 2025
7 checks passed
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.

5 participants