Skip to content

Commit

Permalink
modify Base.Test not to create a closure for each test
Browse files Browse the repository at this point in the history
  • Loading branch information
JeffBezanson committed Jan 27, 2016
1 parent 0318444 commit 6396218
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 37 deletions.
89 changes: 56 additions & 33 deletions base/test.jl
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,17 @@ end

#-----------------------------------------------------------------------

abstract ExecutionResult

immutable Returned <: ExecutionResult
value
end

immutable Threw <: ExecutionResult
exception
backtrace
end

# @test - check if the expression evaluates to true
# In the special case of a comparison, e.g. x == 5, generate code to
# evaluate each term in the comparison individually so the results
Expand All @@ -139,6 +150,7 @@ Returns a `Pass` `Result` if it does, a `Fail` `Result` if it is
`false`, and an `Error` `Result` if it could not be evaluated.
"""
macro test(ex)
orig_ex = Expr(:quote,ex)
# If the test is a comparison
if isa(ex, Expr) && ex.head == :comparison
# Generate a temporary for every term in the expression
Expand All @@ -155,46 +167,52 @@ macro test(ex)
# original comparsion. The block returns
# - an expression with the values of terms spliced in
# - the result of the comparison itself
push!(comp_block.args, Expr(:return,
:( Expr(:comparison, $(terms...)), # Terms spliced in
$(Expr(:comparison, terms...)) # Comparison itself
)))
# Return code that calls do_test with an anonymous function
# that calls the comparison block
:(do_test(()->($comp_block), $(Expr(:quote,ex))))
push!(comp_block.args,
:( Expr(:comparison, $(terms...)), # Terms spliced in
$(Expr(:comparison, terms...)) # Comparison itself
))
testpair = comp_block
else
# Something else, perhaps just a single value
# Return code that calls do_test with an anonymous function
# that returns the expression and its value
:(do_test(()->($(Expr(:quote,ex)), $(esc(ex))), $(Expr(:quote,ex))))
testpair = :(($orig_ex, $(esc(ex))))
end
result = quote
try
Returned($testpair)
catch _e
Threw(_e, catch_backtrace())
end
end
Base.remove_linenums!(result)
# code to call do_test with execution result and original expr
:(do_test($result, $orig_ex))
end

# An internal function, called by the code generated by the @test
# macro to actually perform the evaluation and manage the result.
function do_test(predicate, orig_expr)
function do_test(result::ExecutionResult, orig_expr)
# get_testset() returns the most recently added tests set
# We then call record() with this test set and the test result
record(get_testset(),
try
if isa(result, Returned)
# expr, in the case of a comparison, will contain the
# comparison with evaluated values of each term spliced in.
# For anything else, just contains the test expression.
# value is the evaluated value of the whole test expression.
# Ideally it is true, but it may be false or non-Boolean.
expr, value = predicate()
if isa(value, Bool)
expr, value = result.value
testres = if isa(value, Bool)
value ? Pass(:test, orig_expr, expr, value) :
Fail(:test, orig_expr, expr, value)
else
# If the result is non-Boolean, this counts as an Error
Error(:test_nonbool, orig_expr, value, nothing)
end
catch err
else
# The predicate couldn't be evaluated without throwing an
# exception, so that is an Error and not a Fail
Error(:test_error, orig_expr, err, catch_backtrace())
end)
@assert isa(result, Threw)
testres = Error(:test_error, orig_expr, result.exception, result.backtrace)
end
record(get_testset(), testres)
end

#-----------------------------------------------------------------------
Expand All @@ -205,27 +223,32 @@ end
Tests that the expression `ex` throws an exception of type `extype`.
"""
macro test_throws(extype, ex)
:(do_test_throws( ()->($(esc(ex))), $(Expr(:quote,ex)),
backtrace(), $(esc(extype)) ))
orig_ex = Expr(:quote,ex)
result = quote
try
Returned($(esc(ex)))
catch _e
Threw(_e, nothing)
end
end
Base.remove_linenums!(result)
:(do_test_throws($result, $orig_ex, $(esc(extype))))
end

# An internal function, called by the code generated by @test_throws
# to evaluate and catch the thrown exception - if it exists
function do_test_throws(predicate, orig_expr, bt, extype)
record(get_testset(),
try
predicate()
# If we hit this line, no exception was thrown. We treat
# this a failure equivalent to the wrong exception being thrown.
Fail(:test_throws_nothing, orig_expr, extype, nothing)
catch err
function do_test_throws(result::ExecutionResult, orig_expr, extype)
if isa(result, Threw)
# Check the right type of exception was thrown
if isa(err, extype)
Pass(:test_throws, orig_expr, extype, err)
if isa(result.exception, extype)
testres = Pass(:test_throws, orig_expr, extype, result.exception)
else
Fail(:test_throws_wrong, orig_expr, extype, err)
testres = Fail(:test_throws_wrong, orig_expr, extype, result.exception)
end
end)
else
testres = Fail(:test_throws_nothing, orig_expr, extype, nothing)
end
record(get_testset(), testres)
end

#-----------------------------------------------------------------------
Expand Down
8 changes: 4 additions & 4 deletions test/read.jl
Original file line number Diff line number Diff line change
Expand Up @@ -177,10 +177,10 @@ for (name, f) in l

for text in [
old_text,
UTF8String(['A' + i % 52 for i in 1:(div(Base.SZ_UNBUFFERED_IO,2))]),
UTF8String(['A' + i % 52 for i in 1:( Base.SZ_UNBUFFERED_IO -1)]),
UTF8String(['A' + i % 52 for i in 1:( Base.SZ_UNBUFFERED_IO )]),
UTF8String(['A' + i % 52 for i in 1:( Base.SZ_UNBUFFERED_IO +1)])
UTF8String(Char['A' + i % 52 for i in 1:(div(Base.SZ_UNBUFFERED_IO,2))]),
UTF8String(Char['A' + i % 52 for i in 1:( Base.SZ_UNBUFFERED_IO -1)]),
UTF8String(Char['A' + i % 52 for i in 1:( Base.SZ_UNBUFFERED_IO )]),
UTF8String(Char['A' + i % 52 for i in 1:( Base.SZ_UNBUFFERED_IO +1)])
]

write(filename, text)
Expand Down

4 comments on commit 6396218

@samoconnor
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @JeffBezanson,
I'm curious, why is it necessary to specify this as a Char array, isn't it implied by Char + Int % Int = Char ?

See JuliaLang/Compat.jl#162 (comment)

@JeffBezanson
Copy link
Member Author

Choose a reason for hiding this comment

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

It shouldn't be necessary; it's a temporary workaround until we can fully free comprehensions from dependence on type inference.

@nalimilan
Copy link
Member

Choose a reason for hiding this comment

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

But did the test work previously? Is this an inference regression? See #14883 which might be related.

(A few words in the commit message would avoid the need for this kind of discussion.)

@JeffBezanson
Copy link
Member Author

Choose a reason for hiding this comment

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

The change is that inference is now deciding not to run on this code.

Please sign in to comment.