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

Add support for JUnit 5 extensions (#57) #67

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

bjansen
Copy link

@bjansen bjansen commented Oct 28, 2020

Update here: #67 (comment)

Notes:
* I didn't copy/adapt the full test suite because the code I put in the abstract classes is already tested by the Rule tests. I only added tests that make sure the builder works and the Extension starts/stops the server correctly.
* I declared the junit-jupiter-api Maven dependency as optional to not force JUnit 5 down the throat of existing projects that use JUnit 4.
* I considered changing the junit dependency to be optional too, but didn't do it because that could possibly break projects that don't pull JUnit 4 explicitly when they upgrade embedded-ldap-junit to the next version.

@coveralls
Copy link

coveralls commented Oct 28, 2020

Coverage Status

Coverage decreased (-76.7%) to 14.8% when pulling 6550282 on bjansen:issue-57-junit5 into d43da04 on zapodot:master.

@bjansen
Copy link
Author

bjansen commented Oct 29, 2020

I realized it might be a better design to split the code into separate modules, like what was done in https://github.com/zapodot/embedded-jms-junit for example. Better separation of concerns, no dual dependency on JUnit 4 and JUnit 5 etc. But that will introduce quite a lot of changes in the project structure. WDYT @zapodot?

@zapodot
Copy link
Owner

zapodot commented Oct 29, 2020

Thanks for your contribution!
You are right: It would be better to split JUnit 4 and JUnit 5 support into two separate modules.

@bjansen
Copy link
Author

bjansen commented Oct 29, 2020

All right, I think I can get it to work without breaking compatibility with older versions (especially the Maven coordinates).

@bjansen
Copy link
Author

bjansen commented Oct 29, 2020

One remaining problem after modularization: most of the unit tests are in embedded-ldap-junit, but most of the code is in embedded-ldap-core, so cobertura reports very low coverage even when most of the code is well tested.

@bjansen
Copy link
Author

bjansen commented Jul 30, 2021

Good news! I moved (actually copied) most of the unit tests from embedded-ldap-junit to embedded-ldap-core, and the coverage is now on par with what's currently on master.

I suppose I could remove most of the tests in embedded-ldap-junit which are now redundant, and only leave what's testing the actual JUnit rule stuff, and not the embedded LDAP server. WDYT @zapodot?

So, to summarize:

  • The project is now split into 3 modules:
    • embedded-ldap-core contains most of the code to setup an embedded LDAP server, and is not tied to any testing framework. It has good code coverage.
    • embedded-ldap-junit is a JUnit 4 rule that starts this embedded LDAP server
    • embedded-ldap-junit5 is a JUnit 5 extension that starts this embedded LDAP server
  • This new version should not break anything in existing projects, because Maven coordinates remain the same (the only difference is that embedded-ldap-junit now has an extra dependency on embedded-ldap-core). Package names and class names remain the same.
  • Now that the project is modularized, it's possible in theory to add support for new testing frameworks that have an equivalent of JUnit's @Rule.

@bjansen
Copy link
Author

bjansen commented Jul 30, 2021

I think there's an extra change that I could do: embedded-ldap-core is now mostly abstract classes, but I could add a Simple/Default implementation that would turn the thing into a standalone module.

People who want to embed a simple LDAP server in their application (outside of the context of unit tests) could simply pull embedded-ldap-core and use its builder to set up their server!

@dzikoysk
Copy link

dzikoysk commented Dec 9, 2021

Any updates on this PR? 🤔

@zapodot
Copy link
Owner

zapodot commented Aug 11, 2022

@bjansen any chance that you can rebase from the master branch?

@jrivard
Copy link

jrivard commented Dec 18, 2022

bump....

@bjansen
Copy link
Author

bjansen commented Dec 26, 2022

I rebased from master, I'll probably squash the commits before merging.

@Erates
Copy link

Erates commented Mar 26, 2024

@bjansen @zapodot Any idea when this could be merged? It's kind of usefull 😉

@bjansen
Copy link
Author

bjansen commented Jun 7, 2024

@zapodot do you think you could find some time to review this PR?

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.

6 participants