Skip to content
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

Add io arguments to code_*, and improve reflection tests #9521

Merged
merged 1 commit into from
Dec 31, 2014

Conversation

timholy
Copy link
Member

@timholy timholy commented Dec 31, 2014

code_native, code_llvm, and code_warntype display their results rather than returning an AST. This allows one to specify an IO argument to control where that output ends up. In turn, this allows one to write better tests for these functions.

For code_warntype, this also adds all-caps emphasis for when Base.have_color == false, and for good measure throws in a more careful disabling of emphasis when printing "raw" Expr objects.

@tkelman, I wonder if this might work around #9501? If so, it would suggest that redirect_stdout and redirect_stderr might have problems.

code_native, code_llvm, and code_warntype display their results
rather than returning an AST. This allows one to specify an IO
argument to control where that output ends up. In turn, this allows
one to write better tests for these functions.
@Keno
Copy link
Member

Keno commented Dec 31, 2014

👍 LGTM, we have io arguments on everything else that prints, so I don't see why these should be special.

@ivarne
Copy link
Member

ivarne commented Dec 31, 2014

+1 from me too. I would even argue that the AST returning versions also could accept a IO argument.

@timholy
Copy link
Member Author

timholy commented Dec 31, 2014

AFAICT, the AppVeyor failures are just timeouts.

timholy added a commit that referenced this pull request Dec 31, 2014
Add io arguments to code_*, and improve reflection tests
@timholy timholy merged commit 28feafd into master Dec 31, 2014
@timholy timholy deleted the teh/code_tests branch December 31, 2014 22:56
@tkelman
Copy link
Contributor

tkelman commented Jan 1, 2015

I sort of dislike the warnings indicate normal behavior: bit.

@timholy
Copy link
Member Author

timholy commented Jan 1, 2015

I know what you mean. Are you wishing we would suppress them altogether? I didn't want to do that given #9501.

@tkelman
Copy link
Contributor

tkelman commented Jan 1, 2015

That's fair. If we can trace that down (more likely Jameson or Keno, though I must say that was an impressive bit of bughunting you managed so far) and come up with a solution we're confident in, then we can eventually suppress them. I am a fan of the silence is golden principle when it comes to tests, though also haven't worked too hard to suppress the known warnings from the tests on Windows. I know those have confused a few newcomers.

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.

4 participants