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

Fixes to reports.py -- don't crash on control characters, ensure directories earlier #5885

Merged
merged 6 commits into from Nov 14, 2018

Conversation

Projects
None yet
3 participants
@gvanrossum
Copy link
Member

gvanrossum commented Nov 11, 2018

Fixes #5884 (both parts).

@gvanrossum gvanrossum requested a review from msullivan Nov 11, 2018

@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented Nov 12, 2018

I don't understand why the tests fail on Windows only, but clearly it's got something to do with my moving around the ensure_dir_exists() calls. Perhaps the temporary directory is relative on Windows but absolute on *nix?

@gvanrossum gvanrossum force-pushed the gvanrossum:fix-reports branch 5 times, most recently from 74ae0d7 to 1c9f008 Nov 12, 2018

@gvanrossum gvanrossum force-pushed the gvanrossum:fix-reports branch from 1c9f008 to 7723238 Nov 13, 2018

@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented Nov 13, 2018

At last! Because of my refactor, the AbstractXmlReporter constructor was failing, and somehow this failure fell into a hole in testcmdline.py: it redirects stderr to stdout, but then it never compared what it read from stdout because it skips that when there are any output files (apparently this is because if there are output files, the expected stdout is "Generated HTML report (via XSLT): /some/path" where /some/path is a temporary directory which is not easy to compare. And it only failed on Windows because it's trying to create a directory named <memory> which is fine on *nix but not on Windows. (Or perhaps creating the directory succeeded but writing the file failed.)

I'll try to fix this too.

@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented Nov 13, 2018

@msullivan This is now officially ready for review. I did expand a bit on the original issue (control characters) but I think the report generator and the test framework are a bit better now.

@@ -418,6 +415,10 @@ def __init__(self, reports: Reports, output_dir: str) -> None:
self.last_xml = None # type: Optional[Any]
self.files = [] # type: List[FileInfo]

# XML doesn't like control characters, but they are sometimes
# legal in source code (e.g. comments, string literals).
control_fixer = str.maketrans(''.join(chr(i) for i in range(32)), '?' * 32)

This comment has been minimized.

@msullivan

msullivan Nov 14, 2018

Collaborator

I didn't know this function!

@msullivan
Copy link
Collaborator

msullivan left a comment

Seems good. Why didn't the thing cause trouble before?

@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented Nov 14, 2018

Why didn't the thing cause trouble before?

A comment containing an odd control character was checked into our S code base last week.

I fixed the test harness because I had a hell of a time debugging why the AppVeyor tests were failing (apparently you can't have a directory named <memory> on Windows, but the test harness was ignoring the traceback).

@gvanrossum gvanrossum merged commit bdc5604 into python:master Nov 14, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gvanrossum gvanrossum deleted the gvanrossum:fix-reports branch Nov 14, 2018

@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Nov 14, 2018

Ah, Windows doesn't allow less than or greater than in file paths. I am surprised that the tests didn't fail once there was an attempt to create the file however...

@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented Nov 14, 2018

I am surprised that the tests didn't fail once there was an attempt to create the file however...

That's why I made the changes to testcmdline.py. It runs mypy in a subprocess, and the subprocess was printing a traceback. But there was logic in testcmdline.py that ignored all output (and the exit status) if at least one output file was expected (i.e. one or more [outfile blah] sections). It then complained it couldn't find the expected output file, but it didn't show the traceback.

I fixed it so it will now fail loudly and report the traceback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment