FundingCircle / jackdaw

A Clojure library for the Apache Kafka distributed streaming platform.
https://fundingcircle.github.io/jackdaw/
BSD 3-Clause "New" or "Revised" License
369 stars 80 forks source link

Restore the fixtures module to the public interface #266

Closed brett-lempereur closed 3 years ago

brett-lempereur commented 4 years ago

This module was part of the public interface of the library, and we depend on it in our integration test suites.

codecov[bot] commented 4 years ago

Codecov Report

Merging #266 (996d732) into master (03707b9) will decrease coverage by 0.80%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
- Coverage   81.22%   80.42%   -0.81%     
==========================================
  Files          41       42       +1     
  Lines        2594     2753     +159     
  Branches      153      157       +4     
==========================================
+ Hits         2107     2214     +107     
- Misses        334      382      +48     
- Partials      153      157       +4     
Impacted Files Coverage Δ
src/jackdaw/test/fixtures.clj 67.29% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 03707b9...996d732. Read the comment docs.

cddr commented 4 years ago

As far as I can tell, it was this PR by @gphilipp that changed things

https://github.com/FundingCircle/jackdaw/pull/235

Seems the main purpose of that PR was to remove the dependency on the kafka server which seems reasonable. Was there a reason for also removing the fixtures from the public interface? Is there a workaround?

99-not-out commented 4 years ago

There was a change made to avoid the need of pulling in the admin interface for Kafka (as it hoiks in Scala etc, and makes the base dependencies fairly complex as a result). This involved not putting the test/ tree in the published JARs. The fixtures NS is very useful though - its fine to move it under src, however no-one will be able to use it without adding the required deps themselves under their test profile - so we should add a note to this effect.

brett-lempereur commented 4 years ago

There was a change made to avoid the need of pulling in the admin interface for Kafka (as it hoiks in Scala etc, and makes the base dependencies fairly complex as a result). This involved not putting the test/ tree in the published JARs. The fixtures NS is very useful though - its fine to move it under src, however no-one will be able to use it without adding the required deps themselves under their test profile - so we should add a note to this effect.

Thanks, would the best place for that be in the README.md, or the documentation for the jackdaw.test.fixture module?

gphilipp commented 4 years ago

... This involved not putting the test/ tree in the published JARs.

Well, the test/ tree is never meant to be included in the published artefact 💁‍♂️. Re-reading the original PR, I realise it's a mistake that the fixture ns ended up being moved into the test directory, as the change didn't require it. We discussed it internally at some point, decided it was not necessary yet the move went in 🤦

Thanks, would the best place for that be in the README.md, or the documentation for the jackdaw.test.fixture module?

I think the best place would be the README.

cddr commented 3 years ago

Re-reading the original PR, I realise it's a mistake that the fixture ns ended up being moved into the test directory, as the change didn't require it. We discussed it internally at some point, decided it was not necessary yet the move went in

Most of the test-machine doesn't need the admin-client. And in fact even the bits that do (e.g. creating test topics) can be done using the rest-proxy these days