Test and fix for issue #354

[mlhetland]

Incorporates @timholy's fix and adds my test.

Note: The test does not properly exercize the added yield() statement.

[timholy]

👍

[mlhetland]

Hm. It seems the checks are failing…? Both warnings about running out of inotify capacity and an undefined rseed…?

[mlhetland]

(Oops, it seems my use of @eval in the commit msg. tagged a user. Sorry!)

[mlhetland]

Well, I can't really make any sense of the check failures, here.

[timholy]

Does it reproduce locally if you do ]test Revise?

[timholy]

The inotify text is expected, part of tests that only run on Linux & Travis. See old passing builds in the Travis history.

[mlhetland]

It works locally – tested that before creating the PR. Also, I don't quite see the link between the errors and the new code/test?

[timholy]

It fails for me locally. Are you sure you tested it with ]test Revise? (The ] meaning pkg-mode.)

[mlhetland]

Yes, I understood. I was sure it worked at some point, but I can see it doesn't, now. (It did work when run from the command-line without ].)

[mlhetland]

The new test succeeds, though, and the failure seems unrelated to entr.

[mlhetland]

Oh, well, I'll do some local debugging :-)

[mlhetland]

Well, at least it seems to be specifically the use of entr that fails – not the call to A354.test() nor any other part of the test (such as the @async stuff). And it fails with both the new and the old version of entr. However, if I run julia test/runtests.jl, it works just fine. And all I get from Pkg is:

ERROR: Package Revise errored during testing

Is there no way of getting the full stack trace?

[timholy]

Definitely a weird error. When I run at the REPL, I see it starting the cleanup phase and then Julia just quits outright, dumping me back to the Linux prompt. Might be a Julia bug. I suspect it's a task problem.

[timholy]

Read about the complexities at JuliaLang/julia#6283 and then change the test so it doesn't try to stop the task.

[mlhetland]

Ah, right. I actually came across that in my quest for how to stop the task without using exceptions. And I didn't look at it thoroughly enough :-}

The reason I didn't want to use exceptions was that I seemed to end up with a compound exception that I had to unpack (similarly to in the other test), though I thought that throwing an InterruptException should work in just terminating entr.

And now, somehow, it does… So unless there's some reason not to, I'll just go with that.

[mlhetland]

Whoah! Seemed to more or less for the early checks, but then triggered a segfault in Julia nightly! Any ideas?

[timholy]

Yay! The segfault may be #344

But I get the same error that shows up in one of the appveyor tests, https://ci.appveyor.com/project/timholy/revise-jl/builds/27252479/job/d20rpoxf78awybn4. Looking at the code, I am a bit surprised it works; I would have thought your implementation would suffer from scope problems. Maybe make result a Ref? I.e., result = Ref(0) and then access it with result[].

[codecov]

Codecov Report

Merging #355 into master will increase coverage by 0.52%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #355      +/-   ##
==========================================
+ Coverage   77.18%   77.71%   +0.52%     
==========================================
  Files          11       11              
  Lines        1188     1189       +1     
==========================================
+ Hits          917      924       +7     
+ Misses        271      265       -6

Impacted Files	Coverage Δ
src/Revise.jl	73% <100%> (+1.47%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0426c89...2887254. Read the comment docs.

[mlhetland]

Hm. Could there still be scope issues, even when using a Ref? I.e., could there be some copying going on, or something? Maybe I'll try to have a global result, instead…

[mlhetland]

OK, trying to move the entr and result assignment out of the @async (beyond what's in entr to begin with; I guess nesting that was an issue to begin with), moving the file modification to a separate task instead. Hopefully a bit cleaner.

And now I remember why the InterruptException didn't work with entr before … it's because I didn't have it in a separate task. So now, once again, that doesn't work. Given that we should only get the correct result assignment if nothing goes wrong, I'm just catching my own error with an indiscriminate catch here. Could do more checking and rethrowing, if you think that's necessary.

[mlhetland]

Hm. And now it stalls (sometimes) – I guess the entrwasn't triggered, then. Maybe I'll try another sleep…? :-P

[mlhetland]

Hm. The stall was on macOS, where you've set mtimedelay to 2.1, so the wait before setvalue should be plenty of time to get entr running? I dunno. Adding a bit more poking at the file, just to see if it helps.

[mlhetland]

Welp, at least now the only issues are the Julia nightly segfaults, as far as I can see. So I guess things are perhaps good to go? (Unless you want to turn off this new test on macOS, like the other entr test; it works, for now, but…)

[timholy]

This indeed looks fine. Thanks for your patience! While the final result is fairly simple, it's pretty amazing how many factors come into place when you combine watching file systems & recompilation.

[mlhetland]

That's fine! Yes, it's indeed a bit tricky – and here we had issues with @async mixed up with it all, I guess :-) And the resulting test is sort of a test we might have wanted for entr to begin with, I guess.