Breaking change: Remove test facilities for downstream users #133
Conversation
These dependencies are used in two ways: During testing, and to be used in tests by dependents of TranscodingStreams. The former reason is addressed by moving them to be test-specific dependencies, the latter should ideally be addressed in the dependents themselves.
|
I'm wondering about what's the best way to do in a similar situation in some other packages. There we want to provide tests for an interface since downstream packages really should have access to a standardized test suite. In your opinion, should downstream packages such as CodecLz4 construct the path to the test/testutils.jl file in their tests and include it manually (e.g., in https://github.com/JuliaIO/CodecLz4.jl/blob/092191810862b4b94fe44a939e39e1095013013f/test/frame_compression.jl#L2)? That seems a bit inconvenient for downstream packages. Maybe the package should provide some functionality for making it easier to Maybe an extension with test utilities could be an alternative (with a weak dependency on Test - or maybe that's not possible?)? Or a subpackage? I'm mainly curious about your general opinions, I don't want to change this PR since I don't work with TranscodingStreams 🙂 |
|
We should merge this. Any reason not to at this point? |
|
FYI in AbstractFFTs we made Test a weak dependency for now, but it was not completely clear if a separate subpackage would be the better option. Hiding the test utilities in |
|
Pinging @StefanKarpinski since he also opened #100. |
|
This was fixed by #152 |
Right now, TranscodingStreams.jl (TS) provides test facilities in
src/testtools.jlto be used by downstream packages. An unfortunate consequence of this is that TS now depends on Test and Random, for no good reason. (See #100)This change makes these dependencies test-only dependencies. Not that this is a breaking change, because dependencies on TS right now may use TS's test facilities in their own tests. However, this change will only break test code, not TS usage "in the wild".