Skip to content

Remove testapprox dependency and Reduce the number of test cases for display (#133) #135

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
Nov 19, 2019

Conversation

kimikage
Copy link
Collaborator

@kimikage kimikage commented Nov 7, 2019

This fixes #133.
This does not change the actual test cases except one wraparound case, and also does not change testtrunc.

@johnnychen94
Copy link
Collaborator

johnnychen94 commented Nov 7, 2019

This thwarts the selective execution of tests and causes an error(UndefVarError: testapprox not defined) in certain environments (e.g. Julia v1.0.3 32-bit ARMv7).

I guess this is caused by the outmost test group normed and fixed

@testset "normed" begin
include("normed.jl")
end
@testset "fixed" begin
include("fixed.jl")
end

If I understand it correctly, testapprox is a local function in group normed, using it in fixed should throw an undef error. So I think moving these functions outside of it might fix this issue. (For example, moving them into test/utils.jl.)

For simple things like this, it's all okay here. Just breaking this function and doing copy&paste things doesn't look good to me...😢

@kimikage
Copy link
Collaborator Author

kimikage commented Nov 7, 2019

Just breaking this function and doing copy&paste things doesn't look good to me...

I generally agree with you. However, the function is not a so-called utility function and is nothing but an implementation of tests.
If some tests can be shared between Fixed and Normed, it means that they are tests for FixedPoint, which specify the behavior of FixedPoint. I think what we should share is not the content, but the outside (i.e. testset). However, it goes against the current style of providing independent testsets of Fixed and Normed.

This is off topic, but I think that sharing the implementation should be prioritized over sharing the test.
master...kimikage:commonize

@johnnychen94
Copy link
Collaborator

johnnychen94 commented Nov 8, 2019

It makes sense.

I don't have a strong opinion on this, and just want to point out an alternative here. I believe you're aware of what you're doing. 😄 Thank you for digging into the details and taking care of this package.

P.S. I don't have a solid CS background, and most of the issues you found are really out of my abilities so I'm afraid I can't give many informative comments to help. 😢

@kimikage
Copy link
Collaborator Author

kimikage commented Nov 8, 2019

I am a little educated in electrics, but not educated in computer science or image processing.:sweat_smile:

If I understand it correctly, testapprox is a local function in group normed, using it in fixed should throw an undef error.

As a matter of fact, I have not clarified the cause. I got tired of seeing "Error During Test at test/normed.jl" while debugging, but have never seen the "UndefVarError" on my x86 machines.
In fact, testapprox has no local keyword.

Notably missing from this table are begin blocks and if blocks which do not introduce new scopes.

https://docs.julialang.org/en/v1/manual/variables-and-scoping/

Of course, whatever the cause, your alternative plan should solve the problem correctly!

@johnnychen94
Copy link
Collaborator

johnnychen94 commented Nov 8, 2019

Notably missing from this table are begin blocks and if blocks which do not introduce new scopes.

Hmmm, yes it is, but here I think it's @testset that introduces a new scope here,

# test.jl
using Test

@testset "Foo" begin
    function foo(x)
        println(x)
        return x
    end
    foo(1)
end

foo(2)
» julia --startup=no test.jl 
1
Test Summary: |
Foo           | No tests
ERROR: LoadError: UndefVarError: foo not defined
Stacktrace:
 [1] top-level scope at /Users/jc/Desktop/test.jl:11
 [2] include at ./boot.jl:328 [inlined]
 [3] include_relative(::Module, ::String) at ./loading.jl:1094
 [4] include(::Module, ::String) at ./Base.jl:31
 [5] exec_options(::Base.JLOptions) at ./client.jl:295
 [6] _start() at ./client.jl:464
in expression starting at /Users/jc/Desktop/test.jl:11

However, if we follow the same structure and use include here, it's another story:

# runtest.jl 
using Test

@testset "Foo" begin
    include("foo.jl")
end

foo(2)
# foo.jl 
function foo(x)
    println(x)
    return x
end

@test foo(1)==1
» julia --startup=no runtest.jl 
1
Test Summary: | Pass  Total
Foo           |    1      1
2

I'm confused about this now, although this is off-topic.

@kimikage
Copy link
Collaborator Author

kimikage commented Nov 8, 2019

I am sorry for misleading you.
@testset encloses the tests in a try block, i.e. introduces a new scope. Usually, we don't have to be aware of this, but include works with the (implicitly) specified Module.
That is all I understand about this. 😥

@kimikage
Copy link
Collaborator Author

I submitted a new issue #139, but this PR is not staled. You may merge or close this PR if you want to.

@timholy timholy merged commit 8d17739 into JuliaMath:master Nov 19, 2019
@timholy
Copy link
Member

timholy commented Nov 19, 2019

Thanks!

@kimikage kimikage deleted the issue133 branch November 19, 2019 12:16
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.

testapprox should be moved or inlined
3 participants