Library does not lend itself to unit testing about nredisgraph

[tombatron]

Pull requests are welcome!

[tombatron]

So I've reviewed this question, and the companion pull request, and I have some observations.

In the example test provided above, it appears as if two questions are under consideration:

1. Does the repository execute a graph query and get an expected response?
2. Does the query that the repository sends to RedisGraph match the expected query that (I'm assuming) is defined in the repository?

The first area under test seems better suited to an integration test with appropriate test data. My thinking here stems from the structure of that part of the test. If you notice, it looks like the mocking framework is being exercised and not application logic. In this case, an integration test would allow you to validate that the graph query defined is syntactically correct and returns an expected result set which would obviate the need for the second area of the unit test (which is more of an assertion of the implementation rather than a test of logic).

With all of that said, I really appreciate your contribution but I don't think the suggested changes are appropriate for this library.

Cheers,

Tom

[cidthecoatrack]

I agree that this absolutely would work better as a low-level integration test - that would in fact be my normal way to test a repository like this. However, in this particular circumstance, that isn't an option. The particulars of the test aside, the fact that the library is not abstractable makes unit testing anything that uses it very difficult. So even if I wasn't going to test graph directly as I do in my example test, because I cannot inject/abstract RedisGraph (without wrapping it in my own adapter/factory, as I have done as a mitigating measure), the entire class becomes untestable, which is simply a bad architecture.

Ultimately, it's your choice, as it's your library/package and therefore your architecture. I appreciate the time you've spent looking at it.