Make runner.create_output_path a member of TestRunner class

[LukasVik]

Background: I am working on a multi-threaded build runner for vivado and formal (symbiyosys) projects. Since e.g. Vivado will realistically probably only use about four threads, parallelization of builds is desired. The idea is to re-use the VUnit test runner code, which is robust and tested, but with different "test" objects. However I wish to modify some of the behavior, namely in this case the hashing of test output folder names. To do this I inherit the TestRunner class and override the methods who's behavior I want to change. In this case however, the create_output_path function is not a member of the class, but a bare function.

This PR makes the create_output_path a private member of the TestRunner class. That enables my use case, and is also in my opinion more logical given how it is used by TestRunner. There is no functional change for VUnit.

The second commit changes the name from create_output_path to get_output_path. In my mind, "create" as a verb implies that the directory is created on disk, which is is not.

[eine]

Overall, I think the change makes sense. However, there are a couple of details which I'd like to understand prior to merging:

• Why is it changed from a "public" function/method to a "private" method (preprended with _)?

• I agree that get or construct are better that create. Just because of curiosity, did you check/find when/where is the directory actually created?

[LukasVik]

Why is it changed from a "public" function/method to a "private" method (preprended with _)?

I noticed that it is being used only internally in the class. I think it is good practice to have a minimal public API, and hence functions are private unless they NEED to be public. Then again, I do not really have a stake in this matter. If you think differently, I would be happy to change to a public function.

I agree that get or construct are better that create. Just because of curiosity, did you check/find when/where is the directory actually created?

It is done in TestRunner._run_test_suite with the call to ostools.renew_path(output_path). On a side note, this another of the behaviors I have overloaded and changed in my build runner. For unit tests, it make sense to wipe the directory before each run. But for a Vivado build that is often not the case, since you have a project on disk that takes a long time to create and might contain a state that you want to keep. So I have overloaded the TestRunner._run_test_suite an slightly modified it.

[eine]

I noticed that it is being used only internally in the class. I think it is good practice to have a minimal public API, and hence functions are private unless they NEED to be public. Then again, I do not really have a stake in this matter. If you think differently, I would be happy to change to a public function.

Honestly, I don't have any strong feeling about it. Yet, it would be a breaking change. Hence, we might want not to change it, just for API stability reasons. @LarsAsplund what do you think?

It is done in TestRunner._run_test_suite with the call to ostools.renew_path(output_path).

Thanks! I was searching for it as mkdir and such...

On a side note, this another of the behaviors I have overloaded and changed in my build runner. For unit tests, it make sense to wipe the directory before each run. But for a Vivado build that is often not the case, since you have a project on disk that takes a long time to create and might contain a state that you want to keep. So I have overloaded the TestRunner._run_test_suite an slightly modified it.

That makes sense. Is it a large modification? I.e., would it make sense to upstream it as an option?

[LukasVik]

That makes sense. Is it a large modification?

It is a very small modification, the only change is that the call to ostools.renew_path(output_path) is replaced with a function that creates the directory if it does not exist, otherwise does nothing.

I.e., would it make sense to upstream it as an option?

From my perspective it would be very helpful if we could upstream it. As it is now, I have copied (and minimally changed, see above) the TestRunner._run_test_suite from VUnit. This makes the licensing situation for tsfpga a bit messy. If the change could be upstreamed as an option, I would not have to copy any code. That would save me the licensing hassle, and I would not have to maintain 70 lines of copied code.

I will think of a way to add that option in a nice way, that does not make the VUnit class a code smell, and submit a PR. Probably on monday.

Thanks for the suggestion @eine !

[LukasVik]

I moved the ostools.renew_path(output_path) call to a separate method _prepare_output_path that is called by _run_test_suite. With this change my build runner does not copy the 70 lines of the _run_test_suite, but instead only overrides the _prepare_output_path method.

[LukasVik]

Ping @LarsAsplund: Do you have any opinion about @eine's concern regarding API stability?

This PR changes the function vunit/test/runner.py::create_output_path, which is technically a public function, to be a class member vunit/test/runner.py::TestRunner::_get_output_path.

[LarsAsplund]

While it's a public function it's not really part of the user interface so I think it can be removed. There is a risk of course that it has been used somewhere but it doesn't strike me as a very reusable function.

[eine]

Then, we are all set! Thanks Lukas!

[LukasVik]

Great, thanks guys!