Refactor: AssertJ best practices by @timtebeek#1454
Conversation
There was a problem hiding this comment.
Unfortunately, there are many warnings. See https://jenkins.hope.nyc.ny.us/job/docker-shiro-ci/job/main/38/analysis/
|
Hi, @timtebeek |
|
It seems 51 of those warnings are in For comparison, this is what was there before, with What would you recommend we do about such instances? |
|
And once again, proper testing reveals underlying code issues :) working on it |
33c5e3c to
798cd18
Compare
|
Please pay special attention to this commit: 798cd18 I believe this is safe to put into |
Urgh. By definition a breaking change! Maybe put the 3.x label on that commit? It's probably safe though |
|
My understanding of Java generics erasure is that this change is source and binary compatible with the previous, thus not a breaking change... but I could be wrong. |
Since previous code was "raw" class without any parameters, they would have to cast in that case anyway... |
|
I think the problem can be this one about api change: For now user can put any type in the key and we can imagine that he should use a String but we are not sure. I understand the purpose of the best practice but it's not a good idea to update our api because of unit test framework change. |
Yes, that's a real no go and needs to wait until 3. |
|
Yup. As disappointing as this sounds, this one will have to wait |
|
@lprimak just modify your branch to leave that one out and merge it into 2.0.1 Then do another for 3.0.0 with that change. |
|
Just catching up after some travel. Would most of the concerns here go away if we created a custom assertion for That way we would at least get the migration to AssertJ in (thanks again @timtebeek). |
|
(I can volunteer to write the assertion class, if that resolves the issues), it would probably read something like: Shiro.assertThat(factory).containsEnvironment("foo, "bar"); |
Absolutely! Here's a quick recipe run: https://app.moderne.io/results/j2iJu9Xxx/ |
|
Yea, that sounds great! If there are no warnings it's all good by me :) |
|
I have reverted the API change commit. |
|
I started hacking a custom Assertion class up, and IMHO, it's too complex for the value it provides. The value is essentially, working around a valid compiler warning. The better solution for that is doing something like: My current plan is to open an issue to fix the generic type defs, and mark the effected test methods with something like: @SuppressWarning("unchecked") // TODO: Remove this warning when issue #xxxx is resolved
public void someTestMethod() {
// ...
assertThat(env).containsEntry(Context.SECURITY_PRINCIPAL, "foo");
// ...
}Any objections to this approach? Note I do think custom assertion classes could be useful in other places, just in this case. |
|
No objections from me. |
7005107 to
798cd18
Compare
f09191a to
d9c4883
Compare
Introduce AssertJ by @timtebeek
Supersedes #1450 and #1452
Following this checklist to help us incorporate your contribution quickly and easily:
for the change (usually before you start working on it). Trivial changes like typos do not
require a GitHub issue. Your pull request should address just this issue, without pulling in other changes.
[#XXX] - Fixes bug in SessionManager,where you replace
#XXXwith the appropriate GitHub issue. Best practiceis to use the GitHub issue title in the pull request title and in the first line of the commit message.
fixes #XXXif merging the PR should close a related issue.mvn verifyto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.git rebase -i.Trivial changes like typos do not require a GitHub issue (javadoc, comments...).
In this case, just format the pull request title like
[DOC] - Add javadoc in SessionManager.If this is your first contribution, you have to read the Contribution Guidelines
If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement
if you are unsure please ask on the developers list.
To make clear that you license your contribution under the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.