Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Nov 14, 2018
Merged

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

merged 6 commits into from
Nov 14, 2018

Conversation

gvanrossum
Copy link
Member

Fixes #5884 (both parts).

@gvanrossum gvanrossum requested a review from msullivan November 11, 2018 23:02
@gvanrossum
Copy link
Member Author

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 fix-reports branch 5 times, most recently from 74ae0d7 to 1c9f008 Compare November 12, 2018 23:07
@gvanrossum
Copy link
Member Author

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
Copy link
Member Author

@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.

msullivan
msullivan reviewed Nov 14, 2018
View reviewed changes
mypy/report.py
@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't know this function!

msullivan
msullivan approved these changes Nov 14, 2018
View reviewed changes
Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

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

@gvanrossum
Copy link
Member Author

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
@gvanrossum gvanrossum deleted the fix-reports branch November 14, 2018 04:07
@emmatyping
Copy link
Member

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
Copy link
Member Author

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
Reviewers

@msullivan msullivan msullivan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gvanrossum @emmatyping @msullivan