Try to find location of Rscript executable rather than assuming a hard-coded path.

[crantila]

Thanks, this is much better than what I wrote! Except that the test suite fails on Travis because Rscript isn't installed. But why not finish your cleanup and kill the whole RBarChart.RSCRIPT_PATH thing while you're at it. One possibility is to store the path in a hidden instance variable, and only call the new find_Rscript() function in __init__(), but there are other solutions too. To get the tests to pass, the easiest way might be to mock find_Rscript() for the whole RBarChart test suite.

Also, and this super nit-picky, you changed the import order in barchart.py, possibly to get closer to alphabetical order, but it didn't quite make it. I don't know what the tendency is now, but I usually order my imports from least to most specific, and alphabetically within groups. For VIS, that's:

python_stdlib
pandas
music21
vis

It's really not a big deal, but my anal retentive nature couldn't pass up a chance to complain about something so obviously wrong... GOSH!

[maxalbert]

Haha! I can totally relate to the nit-picky comment. ;-) Happy to change the import order to conform with the established convention.

Regarding RSCRIPT_PATH, I was about to eliminate it but then started wondering why an instance variable would make more sense. After all, this is kind of a piece of global information that doesn't depend on the instance, right? it might as well be a global variable (or else we could call find_Rscript() wherever we need the information). I don't really mind either way, and am also happy to make it an instance variable. However, some of the tests are using RBarChart.RSCRIPT_PATH at the moment, so if we remove it then they would still need a way of accessing that information, and it doesn't feel quite right to let them access a hidden instance variable.

[crantila]

That's a good point, we really don't expect the path to be changing between instances. But it's also not a constant any more because the path is in fact determined at runtime. From this perspective, a module-level variable feels right, but that poses several challenges. You'll have to call find_executable() on module import, delaying VIS start-up time, and actually VIS wouldn't even import if Rscript is missing.

What if you did the call to find_executable() in the run() method, just before you need to know the path to Rscript? For the tests, if find_executable() is mocked, you'll already know the return value, so you can use it for the expected Popen calls.

What do you think about this solution?

[maxalbert]

Sounds like a good idea. I have updated the PR accordingly, but I chose to add an optional argument to RBarChart.run() which allows to explicitly specify the path to Rscript. This is useful in any case (if Rscript is present on the system but not in the PATH for some reason), and it allows to pass a fake path in the tests without the need for more low-level mocking. Do you think this is ok?

The only thing I don't understand is why mock_subpro.assert_called_once_with() now requires the argument stderr=-2 for the assertion to pass. I presume this is because /path/to/Rscript doesn't actually point to a real executable, but why was this not needed before?

[crantila]

I'm not a huge fan of a new parameter in run() because, firstly, it breaks the convention, and secondly, if you're doing this instead of mocking something, then how do you plan to test the thing you didn't mock? The point of mocks, even though they can admittedly complicated, is that you can use the exact same code paths in your test suite and "real life." But it's not a big deal in this Experimenter, so I won't insist (and I don't even know why I got involved in this PR to begin with...).

As for stderr=-2, I'll try to figure out what's going on.

[maxalbert]

Fair points. I don't really mind to be honest and am happy to change it to a mock. I guess my personal take is kind of based on the opposite view: if you're mocking something, how do you know that the mock behaves like the real thing? ;-) I'll take a look at switching it to a mock, though, when I get around to it. This might well make the stderr=-2 weirdness go away.