Added tests around IO::watch and IO::unwatch

[DavyLandman]

Codecov pointed out there were not test for them.

Writing the test I found a bug, so also fixed that.

Note the bug is minor, it would sometimes share too many events.

[codecov[bot]]

Codecov Report

Merging #1659 (30d24a7) into main (2cdd87a) will increase coverage by 0%. The diff coverage is 58%.

@@           Coverage Diff            @@
##              main   #1659    +/-   ##
========================================
  Coverage       48%     48%            
- Complexity    5982    6050    +68     
========================================
  Files          669     669            
  Lines        58308   58320    +12     
  Branches      8497    8501     +4     
========================================
+ Hits         28409   28559   +150     
+ Misses       27763   27597   -166     
- Partials      2136    2164    +28     

Impacted Files	Coverage Δ
src/org/rascalmpl/library/Prelude.java	44% <50%> (+1%)	:arrow_up:
...calmpl/test/infrastructure/RecursiveTestSuite.java	81% <100%> (+1%)	:arrow_up:
...almpl/interpreter/result/ListOrRelationResult.java	62% <0%> (-6%)	:arrow_down:
...g/rascalmpl/parser/gtd/location/PositionStore.java	65% <0%> (-5%)	:arrow_down:
...c/org/rascalmpl/interpreter/result/NodeResult.java	68% <0%> (-1%)	:arrow_down:
...ary/lang/json/internal/JSONReadingTypeVisitor.java	72% <0%> (-1%)	:arrow_down:
src/org/rascalmpl/uri/URIUtil.java	37% <0%> (-1%)	:arrow_down:
src/org/rascalmpl/interpreter/result/Result.java	13% <0%> (-1%)	:arrow_down:
...g/rascalmpl/interpreter/result/RationalResult.java	32% <0%> (ø)
src/org/rascalmpl/interpreter/Evaluator.java	26% <0%> (+<1%)	:arrow_up:
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[DavyLandman]

Hmm, something is going wrong with calculating the test coverage (or running tests) as this commit should have increased the test coverage in prelude. not decrease it.

[DavyLandman]

I know this code is hit by the test, so something is going wrong with the merging of test coverages.

[jurgenvinju]

We do have time for other people's bugs.

[jurgenvinju]

Is it possible that our handwritten (parallel) "JUnit" test runners are messing up what is going on here? At least their Descriptions are all wrong and not pointing to the source module anymore for a few years already.

[DavyLandman]

Ok, solved it, the RecursiveTestSuite runner was not detecting plain junit classes. So this IOTest was just never triggered.

[DavyLandman]

ouch, the watches aren't working as expected on OSX :|

[DavyLandman]

Still breaking in Mac OSX. It seems it's an actual bug.

[jurgenvinju]

Good progress. Can we write this test also in Rascal and would it not be simpler? I'm curious about the Mac bug as well.

[jurgenvinju]

What happens with a bigger wait time on Mac?

[DavyLandman]

Can we write this test also in Rascal and would it not be simpler?

We need to sleep outside of the rascal evaluator lock, so the watch thread can get the lock on the evaluator to invoke the callback. So that's why we cannot do this in pure rascal.

What happens with a bigger wait time on Mac?

Could you give that a try on your machine? Maybe indeed the interval that the Mac api triggers is slower than on linux or windows. But finding this out via multiple commits seems a bit wasteful.

[jurgenvinju]

I experimented with the tests and it seems in this case the watchers do work correctly, also on Mac, but the tests are unreliable under certain circumstances. The problem is that the Mac implementation of the watchers is built internally with a polling mechanism that uses timestamps, which behaves differently and can violate the assumption under which these tests have been written.

• One difference in semantics is that the poller may report file changes that are older, from before the watcher was even registered, to the current watcher.

Another related issue is that tests do have impact on each other via:

1. the file system
2. the registry of watchers

This, in combination with the above difference in semantics, can also lead to the current test suite failing for no other reason than the way they were designed.

We could figure out a different way of testing the watchers:

• should not abstract from the location scheme. We should test with file as well as others to get coverage of the different implementations.
• should use clean and unique tmp folders for every test.
• allow for additional changes to be reported by a watcher next to the expected change
• test for specific changes with specific locations rather than counting them

[DavyLandman]

I experimented with the tests and it seems in this case the watchers do work correctly, also on Mac, but the tests are unreliable under certain circumstances. The problem is that the Mac implementation of the watchers is built internally with a polling mechanism that uses timestamps, which behaves differently and can violate the assumption under which these tests have been written.

* One difference in semantics is that the poller may report file changes that are older, from before the watcher was even registered, to the current watcher.

Okay, that is indeed bad. There is an api in osx (WatchService) and there do seem to be libraries that support that via a JNI binding: https://github.com/gmethvin/directory-watcher

Another related issue is that tests do have impact on each other via: 1. the file system 2. the registry of watchers

This, in combination with the above difference in semantics, can also lead to the current test suite failing for no other reason than the way they were designed.

Could you elaborate a bit more? Are you talking about the time between a watch setup and the file events that happen closely thereafter?

We could figure out a different way of testing the watchers: * should not abstract from the location scheme. We should test with file as well as others to get coverage of the different implementations. * should use clean and unique tmp folders for every test. * allow for additional changes to be reported by a watcher next to the expected change * test for specific changes with specific locations rather than counting them

Agreed, but my first goal was to get any testing in there, just so that we're not missing breaking changes when we introduce them.