Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore the fixtures module to the public interface #266

Merged
merged 1 commit into from
Mar 1, 2021

Conversation

brett-lempereur
Copy link
Contributor

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

@brett-lempereur brett-lempereur requested a review from a team as a code owner July 20, 2020 09:46
@codecov
Copy link

codecov bot commented Jul 20, 2020

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
Copy link
Contributor

cddr commented Jul 20, 2020

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

#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
99-not-out previously approved these changes Jul 20, 2020
@99-not-out
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

... 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
Copy link
Contributor

cddr commented Dec 8, 2020

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

This module was part of the public interface of the library, and we
depend on it in our integration test suites.
@gphilipp gphilipp merged commit f8c6546 into master Mar 1, 2021
@gphilipp gphilipp deleted the restore-test-fixtures-module branch March 1, 2021 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants