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

Pkg.test failing, but runtests.jl is O.K. #7256

Closed
garborg opened this issue Jun 13, 2014 · 10 comments
Closed

Pkg.test failing, but runtests.jl is O.K. #7256

garborg opened this issue Jun 13, 2014 · 10 comments
Labels
bug Indicates an unexpected problem or unintended behavior packages Package management and loading

Comments

@garborg
Copy link
Contributor

garborg commented Jun 13, 2014

Creating the a tiny package (detailed below), Pkg.test returns the following error:

julia> Pkg.test("P")
INFO: Testing P
ERROR: f not defined
 in include at boot.jl:244
 in reload_path at loading.jl:152
 in _require at loading.jl:67
 in require at loading.jl:54
 in include at boot.jl:244
 in reload_path at loading.jl:152
 in _require at loading.jl:67
 in require at loading.jl:51
 in include at boot.jl:244
 in include_from_node1 at loading.jl:128
while loading /Users/sean/.julia/v0.3/P/test/JSON.jl, in expression starting on line 1
while loading /Users/sean/.julia/v0.3/P/src/P.jl, in expression starting on line 3
while loading /Users/sean/.julia/v0.3/P/test/runtests.jl, in expression starting on line 1

Including runtests.jl works fine, and so does Pkg.test if I do EITHER of the following:
remove using JSON OR rename the unused (in this minimal example) test/json.jl.

(There is no test/JSON.jl, only test/json.jl.)

Setup:
Pkg.generate("P", "MIT")

Files:
src/P.jl:

module P
using JSON
export f
include("json.jl")
end # module

src/json.jl: f() = "I'm here."

test/runtests.jl:

using P
using Base.Test
println(f())

test/json.jl:

@Keno
Copy link
Member

Keno commented Aug 4, 2014

I assume this is on a mac? If so the difference is probably what directory you're in and that mac has a case-insensitive file system by default.

@garborg
Copy link
Contributor Author

garborg commented Aug 4, 2014

Gotcha -- so case sensitivity plus that Pkg.test() is changing the directory to prefer to load modules from "test/" over elsewhere.

Maybe not undesirable behavior by itself? But people could mock packages/modules other ways, and it's a different directory than users (or at least I) commonly run tests from, so I'd guess it's better to change it rather than restricting test file names.

@IainNZ
Copy link
Member

IainNZ commented Aug 5, 2014

The directory-that-tests-are run thing came up a lot in the earlier days of PackageEvaluator. I believe I run them from the root of the package, rather than the test folder, but I initially ran it from the test folder. I basically harassed people to make it work from the root of the package - but I don't feel like people should rely on that.

@IainNZ
Copy link
Member

IainNZ commented Aug 9, 2014

I think this can be closed

@garborg
Copy link
Contributor Author

garborg commented Aug 9, 2014

I apologize for the delay.I may be missing something, but I'm not sure this should be closed.

My understanding is that the naming of files in the test directory will break Pkg.test for a subset of users, after passing undetected both by Travis (if the developer uses it) and local testing (if the developer uses something like julia test/runtests.jl or isn't on OS X).

If it were consistently caught by common local testing workflows, I think it would be fine to close, but expecting developers to think of the scenario described above each time a test file is added -- that seems like a string of gotchas.

@IainNZ
Copy link
Member

IainNZ commented Aug 9, 2014

I'm not sure how this is a Julia problem though? This isn't a problem that people seem to have hit much in 300+ packages

@IainNZ
Copy link
Member

IainNZ commented Aug 9, 2014

Maybe I'm just not understanding your perspective: can you give another example of the scenario where a problem occurs?

@garborg
Copy link
Contributor Author

garborg commented Aug 9, 2014

I started refactoring/expanding DataFramesIO, one of the formats it deals with is JSON, I tried out Pkg.test on a whim, and it took me an embarrassingly long time to figure out what was causing the failure, and even then, I didn't put together why it failed.

As it stands, this will keep slipping through developer testing and Travis and then looks broken to users who are looking to get involved.

Maybe it's not really an issue, because I'm sure the developer will reach out if they're stumped when it's reported, but I was just thinking it would come out looking like better design if one official testing mechanism didn't rely on something the other official testing mechanism doesn't catch (when they could be aligned to catch it where possible).

I know you've thought a lot about testing and I could be missing something -- just wanted to push a little more in case it was the first time this had come up.

@IainNZ
Copy link
Member

IainNZ commented Aug 9, 2014

Its nothing to do with testing though right? Its all about Julia's include/using rules? I think all Julia code, testing or otherwise, should be written to be robust to where it is run from.

@garborg
Copy link
Contributor Author

garborg commented Aug 9, 2014

Agreed, but unless the include/using rules are expected to change, having Pkg.test run tests from a different directory (from where PackageEvaluator, existing Travis configs, developer documentation, etc. do) when it's known to cause issues for a subset of end users, it's also a Pkg.test issue.

Since everything else uses it and I haven't seen many .jl files in package root directories, I'm guessing there aren't any downsides to outweigh the one upside to changing Package.test here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior packages Package management and loading
Projects
None yet
Development

No branches or pull requests

5 participants