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 more @test tests #22609

Merged
merged 3 commits into from
Jun 30, 2017
Merged

Add more @test tests #22609

merged 3 commits into from
Jun 30, 2017

Conversation

omus
Copy link
Member

@omus omus commented Jun 29, 2017

Minor follow up to #22558.

I also discovered a small issue when invalid syntax is used when calling isapprox or isequal where a MethodError will be thrown instead of a syntax error:

julia> isequal(; a=1:2...)
ERROR: syntax: splicing with "..." cannot be used for a keyword argument value

julia> @test isequal(; a=1:2...)
Error During Test
  Test threw an exception of type MethodError
  Expression: isequal(; a=1:2...)
  MethodError: no method matching Pair(::Symbol, ::Int64, ::Int64)

Due to the way @test revises the expression so that each argument is evaluated I'm not sure much can be done here without evaluating the original expression and the arguments separately (effectively evaluating the arguments twice). I can open an issue if need be.

@omus omus added test This change adds or pertains to unit tests testsystem The unit testing framework and Test stdlib labels Jun 29, 2017

# @test keyword precedence: post-semicolon keyword, suffix keyword, pre-semicolon keyword
@test isapprox(1, 2, atol=0) atol=1
@test isapprox(1, 3, atol=0; atol=2) atol=1
Copy link
Contributor

Choose a reason for hiding this comment

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

this and the previous test should be removed as they produce error
syntax: keyword argument "atol" repeated in call to "Base.Test.Expr"

Copy link
Member Author

@omus omus Jun 29, 2017

Choose a reason for hiding this comment

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

Can you give an example where this is giving a syntax error? What version of Julia did you try this on?

julia> using Base.Test

julia> @test isapprox(1, 2, atol=0) atol=1
Test Passed

julia> @test isapprox(1, 3, atol=0; atol=2) atol=1
Test Passed

Copy link
Member

Choose a reason for hiding this comment

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

I can reproduce @DrKrar's concern at the REPL

julia> versioninfo()
Julia Version 0.7.0-DEV.703
Commit 4226320 (2017-06-24 18:23 UTC)
Platform Info:
  OS: macOS (x86_64-apple-darwin15.6.0)
  CPU: Intel(R) Core(TM) i7-3520M CPU @ 2.90GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Sandybridge)
  LAPACK: libopenblas64_
  LIBM: libopenlibm
  LLVM: libLLVM-3.9.1 (ORCJIT, ivybridge)
Environment:
  JULIA_EDITOR = mate

julia> using Base.Test

julia> @test isapprox(1, 2, atol=0) atol=1
ERROR: syntax: keyword argument "atol" repeated in call to "Base.Test.Expr"

julia> @test isapprox(1, 3, atol=0; atol=2) atol=1
ERROR: syntax: keyword argument "atol" repeated in call to "Base.Test.Expr"

Rebuilding master now, will update afterwards.

Copy link
Member

Choose a reason for hiding this comment

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

Works on up-to-date master, so chances are @DrKrar's concern applies to a slightly out-of-date build. Best!

julia> versioninfo()
Julia Version 0.7.0-DEV.796
Commit 2c85595 (2017-06-29 16:32 UTC)
Platform Info:
  OS: macOS (x86_64-apple-darwin15.6.0)
  CPU: Intel(R) Core(TM) i7-3520M CPU @ 2.90GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Sandybridge)
  LAPACK: libopenblas64_
  LIBM: libopenlibm
  LLVM: libLLVM-3.9.1 (ORCJIT, ivybridge)
Environment:
  JULIA_EDITOR = mate

julia> using Base.Test

julia> @test isapprox(1, 2, atol=0) atol=1
Test Passed

julia> @test isapprox(1, 3, atol=0; atol=2) atol=1
Test Passed

Copy link
Member Author

@omus omus Jun 29, 2017

Choose a reason for hiding this comment

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

Gotcha. You definitely need the hot out of the oven 0.7.0-DEV.791 (82df8e7) or above for these tests to work.

Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

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

superficially lgtm! :)

@tkelman tkelman merged commit 71d7c16 into master Jun 30, 2017
@tkelman tkelman deleted the cv/more-test-tests branch June 30, 2017 15:03
@omus
Copy link
Member Author

omus commented Jun 30, 2017

Thanks Tony

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test This change adds or pertains to unit tests testsystem The unit testing framework and Test stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants