Skip to content

Fix Windows path regex escaping in MainModuleTest.testArgPassing - #679

Open
Ankush432-cloud wants to merge 1 commit into
google:masterfrom
Ankush432-cloud:fix/windows-regex-escaping
Open

Fix Windows path regex escaping in MainModuleTest.testArgPassing#679
Ankush432-cloud wants to merge 1 commit into
google:masterfrom
Ankush432-cloud:fix/windows-regex-escaping

Conversation

@Ankush432-cloud

Copy link
Copy Markdown

assertOutputMatches() interprets its expected output as a regular expression. On Windows, os.path.join() returns paths with backslashes, which can be interpreted as invalid regex escape sequences (e.g. \p). Escape the expected path with re.escape() before passing it to assertOutputMatches(), making the test portable across platforms.

@google-cla

google-cla Bot commented Jul 11, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@Ankush432-cloud

Ankush432-cloud commented Aug 22, 2026

Copy link
Copy Markdown
Author

Thanks @lucassouzaalff-lang for working on the testFileNameFire Windows fix and for connecting it with #693 and #679.

I had actually noticed the same testFileNameFire Windows issue while I was investigating the Windows-specific test failures. Since I was already working on #679, I was planning to wait for that PR to move forward and then work on this fix separately. So it was great to see that you had already picked it up — and also that @MohammedAlkindi had identified and documented the issue in #693. Great catch, Mohammed!

I also noticed that #695 currently has the cla/google check failing because of the missing CLA, along with a workflow awaiting maintainer approval. If #695 becomes blocked or you decide not to continue with it, would you be open to letting me take over the testFileNameFire fix and submit it as a separate PR? I’d really appreciate the opportunity to contribute this fix as well.

Of course, if you’re planning to continue with #695, no worries at all — I’ll leave the fix with you. I just wanted to ask because I had already been investigating the same issue and would be happy to take it forward if needed.

Also, @MohammedAlkindi, thank you for identifying and documenting these Windows-specific issues in #693 and for pointing out that #679 addresses the testArgPassing failure. I really appreciate you connecting the two fixes!

@MohammedAlkindi

Copy link
Copy Markdown

Verified on Windows 11 Home (build 26200), Python 3.13.13 and 3.14.7. python -m pytest fire/ -q at master 716bbc2: 2 failed, 259 passed. With this PR: 1 failed, 260 passed, the remaining failure being testFileNameFire, which #695 covers. Both applied: 261 passed.

The build job has not run on this PR either, so re.escape has no CI signal behind it. The matrix is macos + ubuntu, and Windows is the only platform where the unescaped \p is reachable.

@Ankush432-cloud

Copy link
Copy Markdown
Author

@MohammedAlkindi @dbieber Since the Windows-specific failure is confirmed and the current CI matrix doesn't include Windows, would you recommend keeping this PR as a separate fix, or is there anything else you'd like me to add/change before it can be considered for merging?

@MohammedAlkindi

Copy link
Copy Markdown

Separate is right. The two failures are independent: this PR fixes testArgPassing only and #695 fixes testFileNameFire only, and in my Windows run each one left the other's test failing exactly as it was before, so neither depends on the other. Merging them would also park your passing cla/google check behind the one on #695 that is still failing.

Nothing I would change in the diff itself. Whether a windows-latest leg joins the matrix is @dbieber's call and doesn't need to hold this PR up.

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