-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Refactor: AssertJ best practices by @timtebeek #1454
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, there are many warnings. See https://jenkins.hope.nyc.ny.us/job/docker-shiro-ci/job/main/38/analysis/
Hi, @timtebeek |
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 :) |
…d warnings" This reverts commit 798cd18.
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 onjections from me. |
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
#XXX
with 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 #XXX
if merging the PR should close a related issue.mvn verify
to 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.