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

Added default executor modifiable for tests. #15

Closed
wants to merge 1 commit into from

Conversation

VoyTechnology
Copy link
Member

@VoyTechnology VoyTechnology commented Nov 13, 2019

The executor abstraction allows us to introduce a mock executor, which removes
the need for VBoxManage installed on the box, as well as introducting various
failure scenarios which could then be tested and improve the reliability of the
project.

With the current implementation, the mock executor always returns nil, and
allows all tests to pass. This behaviour should be changed in the future,
perhaps adapted even more with closures to allow the list of items that should
be returned as output.

In its current state, the function signature should remain the same.

The executor abstraction allows us to introduce a mock executor, which removes
the need for VBoxManage installed on the box, as well as introducting various
failure scenarios which could then be tested and improve the reliability of the
project.

With the current implementation, the mock executor always returns nil, and
allows all tests to pass. This behaviour should be changed in the future,
perhaps adapted even more with closures to allow the list of items that should
be returned as output.
@VoyTechnology
Copy link
Member Author

Related to #14. Not sure does it actually resolve the problem.

@ringods
Copy link
Contributor

ringods commented Nov 13, 2019

@VoyTechnology last week, I have been digging into the graph of forks for this repository and found a fork which had a full mock including tests already available. I won't have time today to look it up again, but I can let you know by tomorrow. If you would have time already, have a look into the list of forks (https://github.com/terra-farm/go-virtualbox/network).

@VoyTechnology
Copy link
Member Author

I am not 100% sure, but I think https://github.com/asnowfix/go-virtualbox has the most complete fork.

@asnowfix How do you feel about merging your changes upstream?

@asnowfix
Copy link
Contributor

No objection to propose my changes upstream... provided one can tell which repo is considered the active upstream one!

@VoyTechnology
Copy link
Member Author

This was the original repo, forked by many, and then donated to terra-farm. To my best knowledge this is the original repo. The issue to track pulling in changes is #7. If the changes are merged I would close this PR

@ringods
Copy link
Contributor

ringods commented Nov 30, 2019

@VoyTechnology see #17 for the merge of the changes of @asnowfix into this repository. OK if we close this PR?

@ringods
Copy link
Contributor

ringods commented Dec 4, 2019

@VoyTechnology I have merged the mock setup from @asnowfix from #17. Closing this PR.

@ringods ringods closed this Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants