Open craig0990 opened 1 month ago
I see, this is an interesting edge-case.
I can think of 2 ways of solving this:
@omermorad what are the pros/cons of each? What values should we look at for comparison?
Thinking just a bit more, I see a concern about the exposed concreteImplementation
potentially drifting from the unit under test after changes/refactors. I haven't immediately checked, but I assume the Suites Warning: Unreachable Exposed Dependency Detected
warning wouldn't catch that, because it's based on the identifier, not the concrete implementation.
I'm 50/50 myself - I like the zero-setup simplicity of Suites, and manually applying exposed dependencies for tokens feels like the simplest extension of that (and hopefully, real-world scenarios only have a "few" exposed dependencies, rather than dozens).
Exposing an instance of the DI container feels safer; but I'm unsure about the complexity it brings in - different container interfaces, composed container modules, etc.
Tricky one, perhaps 🥲
Edit: the only alternative thought I've had is a change to the API proposed in #465 to something like:
testbed
.expose(ClassAsToken) // As it works now
.exposeToken(Symbol('MY_TOKEN'), MyTokenImplementation) // #465 style approach
.exposeContainer(container) // as @soryy708 proposed
That "might" be a clearer API, without needing overrides, allowing individual method signatures to evolve if necessary. Would probably need some more thought (.exposeContainer()
should probably only be called once, for example). But that's the only other idea I've had vs. the PR currently raised.
@craig0990 ,
I’ve spent quite a bit of time reflecting on your feedback and the ideas we discussed during our video call, and it really helped me gain clarity on how I envision Suites' API and testing philosophy.
The feedback you provided about using tokens in .expose()
sparked a deeper reflection on what Suites is trying to achieve, especially when it comes to sociable unit tests. I’ve come to realize that the solution here isn’t so much about adding more functionality, but rather about clarifying the intended approach and refining the documentation to guide developers in the right direction.
.expose()
Allowing tokens within .expose()
could inadvertently encourage patterns that blend dependency injection (DI) and dependency inversion (DIP) without developers being fully aware of the implications. This blurring of lines runs counter to the simplicity and clarity we want to maintain within Suites.
The core idea behind sociable unit tests is to expose real classes and verify their interactions. Using class-based injection in .expose()
keeps the test context straightforward and aligned with the framework's intended purpose. Tokens are great when genuine inversion of control is needed, but for those cases, I’m advocating the use of .mock().impl()
or .mock().final()
(and test the concrete implementations using .solitary()
approach).
This distinction is crucial. Here’s what I’ve landed on:
DI without Tokens: When the goal is to test real class interactions without entering the realm of inversion, concrete class injection in .expose()
is the way to go. It maintains simplicity and ensures a clear testing context.
DIP with Tokens: If you’re abstracting behavior or decoupling implementations using tokens, this is where dependency inversion principles come in. In such cases, solitary unit tests should be employed to verify different concrete implementations. For sociable tests, I suggest using .mock().impl()
or .mock().final()
to simulate real behavior without exposing tokens directly.
.exposeContainer()
and Token-Based ExposureI believe introducing those functionalities would deviate from the core goal of keeping Suites clear. Instead, I’m leaning towards doubling down on the education and documentation side. I think we should add more detailed guidelines and explanations to the docs, covering:
.expose()
and its limitations regarding tokens..mock().final()
and .mock().impl()
in cases of dependency inversion.\ To summer up, I feel the solution is more about establishing clear rules and documentation around the product. This way, developers can adhere to a consistent testing philosophy that promotes better, more maintainable tests.
What do you think? @soryy708 I would love to here your opinion as well :)
Allowing tokens within .expose() could inadvertently encourage patterns that blend dependency injection (DI) and dependency inversion (DIP)
I don't follow. Can you provide a concrete example?
I feel the solution is more about establishing clear rules and documentation around the product
If that's the direction you want to take, then I propose adding a check inside Suites for whether the DI container contains tokens that weren't .mock().impl()
ed, and log it as a warning.
I think I understand the argument from @omermorad. If I tried to summarize it (probably poorly) for myself from the Inversify perspective I started with:
symbol | string
) vs. the interface. And providing some bound implementation in the TestBed in that case is ... questionable, since it implies/creates some coupling between concrete implementations that the general point of DIP is to avoid and genericizeIn my scenario (e.g., classes using mediators or caches, which might benefit from sociable testing), this setup might require more boilerplate to establish explicit mocks instead of reusing existing concrete implementations. But if I genuinely need to verify interactions between two implementations, it might be better to avoid symbol|string
identifiers in the first place as it implies a separation that my tests aren't really enforcing.
I can understand that Suites would prefer to keep those concepts very clear, so no complaints from me to close this @omermorad.
The main point of DIP is to enable shifting what we depend on from volatile things to stable ones. So instead of something that doesn't change much depending on something we want to be able to change often (and therefore making it harder to change), we can invert the dependency so that the thing that changes often depends on the thing that doesn't.
We have 3 domains we're looking at: The tests suite, the system under test, and the DI framework/container. What's stable and what's volatile? Who should depend on who?
Is there an existing issue for this?
457
Current behavior
I have the same issues as #457, but that issue is currently closed, so I want to add my 2 cents here and offer a possible solution.
When using Inversify with token-based injection (I'm using Symbols, specifically), and trying to expose a class for a
.sociable()
test, I get a warning, and my class does not appear to be exposed :unitRef
myIdentifier as never
to workaround theType
type error.expose<MyTargetClass>(...)
, implied in the same workaround comment, but this does not compile for me becauseexpose
is not actually a generic methodAfter looking around the Suites implementation, I don't see how this could even be possible, at the moment.
The
this.classesToExpose
check in thedependency-resolver.ts
filters onidentifier
values withtypeof identifier === "function"
.Event if it didn't, I don't quite understand how Suites would be able to figure out what the exposed/concrete implementation should be when using token based injection. When using classes as tokens, like NestJS, it's 1-1 and the concrete implementation is the same as the identifier.
But with a symbol or string token, what is Suites supposed to perform a lookup against? The application-level container(s) aren't part of the TestBed.
Minimum reproduction code
https://github.com/craig0990/suites-sociable-inversify
Steps to reproduce
No response
Expected behavior
I'd like to suggest that the
expose()
method should accept a second parameter when the identifier is a string or symbol token, to use as a concrete implementation of the dependency:I've taken a fork to see what it would take to implement this, and have a possible solution in #465. I understand this PR will be proposing changes out of the blue, but if nothing else it could provide some options.
Suites version
3.0.0
Node.js version
21.3.0
In which operating systems have you tested?
Other
No response