Skip to content

Actaully test recursive_unitless_eltype #56

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

Merged
merged 1 commit into from
Jun 28, 2019
Merged

Actaully test recursive_unitless_eltype #56

merged 1 commit into from
Jun 28, 2019

Conversation

janlucaklees
Copy link
Contributor

@janlucaklees janlucaklees commented Mar 17, 2019

While working on #55 I noticed, that the tests for the
recursive_unitless_eltype function are not actually run. They are
written down, but without a @test macro their result are never
checked. So I added the macro.

While working on #55 I noticed, that the tests for the
`recursive_unitless_eltype` function are not actually run. They are
written down, but without a `@test` macro their result are never
checked. So I added them.
recursive_unitless_eltype(AofuSA) == SVector{2,Float64}

@inferred recursive_unitless_eltype(AofuSA)
@test recursive_unitless_eltype(AofuSA) == SVector{2,Float64}
Copy link
Member

Choose a reason for hiding this comment

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

@test @inferred(recursive_unitless_eltype(AofuSA)) == SVector{2,Float64}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @YingboMa, I appreciate your comment, but could you please elaborate?
If you mean to tell me that I would need to add the @inferred back, please explain to me why.
From my understanding, the @inferred is just another way of testing things. But as I added the @test macros, I think we don't need it anymore.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry about that. I should have explained. @inferred is a stronger test than @test recursive_unitless_eltype(AofuSA) == SVector{2,Float64}. Consider the following case

julia> using Test

julia> foo() = rand() > 2 ? Int : Float64
foo (generic function with 1 method)

julia> @test foo() == Float64
Test Passed

julia> @inferred foo()
ERROR: return type Type{Float64} does not match inferred return type Union{Type{Float64}, Type{Int64}}
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] top-level scope at none:0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @YingboMa, thank you for the clarification! Always nice to learn :) As I understand it now, @inferred is used to ensure type stability.
Would it make sense to add @inferred to all tests?

Copy link
Member

Choose a reason for hiding this comment

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

as many as possible, yes it would be good to have on any test that's checking output types.

@ChrisRackauckas ChrisRackauckas merged commit 802d6a7 into SciML:master Jun 28, 2019
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.

3 participants