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

Make runner.create_output_path a member of TestRunner class #682

Merged
merged 3 commits into from
Sep 30, 2020
Merged

Make runner.create_output_path a member of TestRunner class #682

merged 3 commits into from
Sep 30, 2020

Conversation

LukasVik
Copy link
Contributor

@LukasVik LukasVik commented Sep 23, 2020

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
Copy link
Collaborator

eine commented Sep 23, 2020

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
Copy link
Contributor Author

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
Copy link
Collaborator

eine commented Sep 24, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

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 eine merged commit 151acb0 into VUnit:master Sep 30, 2020
@eine
Copy link
Collaborator

eine commented Sep 30, 2020

Then, we are all set! Thanks Lukas!

@LukasVik
Copy link
Contributor Author

LukasVik commented Oct 1, 2020

Great, thanks guys!

@LukasVik LukasVik deleted the output_path branch October 1, 2020 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants