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

setEngineServiceProvider should either be made obsolete or public #50

Open
Derongan opened this issue Nov 18, 2021 · 3 comments
Open

setEngineServiceProvider should either be made obsolete or public #50

Derongan opened this issue Nov 18, 2021 · 3 comments

Comments

@Derongan
Copy link
Member

Any test cases requiring the geary engine need to call this test method or implement something similar.

This should be exposed.

Additionally, the fact that using a manually constructed concrete implementation of the geary engine requires a service provider is kind of silly. This should be refactored away.

@0ffz
Copy link
Member

0ffz commented Nov 20, 2021

This was essentially done because accessing the Engine class directly is the intended way in code. Beyond reworking everything to use DI this service method is the only thing we can do.

@Derongan
Copy link
Member Author

It may be the intended way in code, but this isn't what a unit test is for. You should test using the constructed instance.

When you want to test that the Engine getter is working as intended, you should make a test for that class instead. There is some overlap in my complaints here from #52.

If for some reason this is impossible (which it really shouldn't be) then the setEngineServiceProvider testing utility should be made public as it seems that it is completely required for testing anything.

@Derongan Derongan reopened this Nov 20, 2021
@Derongan
Copy link
Member Author

I tested this just now with my own unit tests, and they fail unless the provider is set up. This is very confusing. The engine service provider looks like it is part of the API to expose the engine to say for example paper users. It shouldn't be something relied on by the engine itself.

Right now if I construct two engines, only the one registered in the service provider is actually usable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants