search

Luca3317 / OpenLibrary.NET

C# library for OpenLibrary by the InternetArchive
MIT License
15 stars 4 forks source link

IOpenLibraryClient interface + loader interfaces #13

Closed JeCFe closed 4 months ago

JeCFe commented 4 months ago

Added example tests to prove that the new IOpenLibraryClient could be correctly handled and mocked using tools such as Moq.

Additionally, added interfaces for the loaders, as these fell under the initial same issue as OpenLibraryClient where base class implementation could not have their method implementations mocked.

Happy for discussions around whether the examples tests I added in should be added into the main branch, or more happy as a proof of concept that the implemented changes has worked as intended but provide no value in main. Or whether additional test of all the loaders, and other methods would be beneficial.

I have been able to successfully run the new tests I added, but have been unable to run the original tests in the project (Issue existed before any changes were made). So validating these tests are still passing green from a maintainer that is able to run these tests would be amazing!

Luca3317 commented 4 months ago

I just cloned your fork and reran all of my original tests, all passed (with the exception of two OLWork tests due to some OLEditionData checks, just pushed the updated versions). Did you run into any other issues running the tests besides this? 503's are also pretty common cause of tests failing.

As for the example tests you added, since they really only test Moq behavior (unless I missed something), I don't think they have much of a purpose on main. If you disagree please let me know.

JeCFe commented 4 months ago

I had issues building large parts of the code based locally due to the dependency references in the csproj. Resolved this locally in the end, but did hamper the easy building and test running

I absolutely agree about the example tests. I don't see there being much value with these existing on the main line. I added them primarily for confidence that the interface implementation was correct for mocking.

Luca3317 commented 4 months ago

I had issues building large parts of the code based locally due to the dependency references in the csproj.

Ah, I see. If you have the time, feel free to open an issue for the specific issues you were having. I made a little effort to make the relative paths of the package references neat but never tested on another machine

Luca3317 commented 4 months ago

Went ahead and cherry-picked the relevant commits 👍