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

Factor out some common object mocking in tests #15396

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

gzhao9
Copy link

@gzhao9 gzhao9 commented Jul 11, 2024

Hi there,

This PR is to address issue #14768, which involves reducing repeated mock object creation in tests.

I mistakenly closed PR #15256 and then discovered that I do not have permission to reopen it, receiving the message this branch was force pushed or recreated. I'm not sure if I should reopen the original or create this new PR

Although my issue submission mentioned 4 separate draft PRs, I combined them into this PR, since they all address the same problem.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 11, 2024
@gzhao9
Copy link
Author

gzhao9 commented Jul 11, 2024

@jesperronn Please feel free to let me know if there is anything I need to change. I look forward to your suggestions.

Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup, @gzhao9. I think there is already a MockSecurityContextHolderStrategy available in spring-security-core. Can you please enhance that instead?

I think it would be reasonable to add the default constructor and one that takes a SecurityContext to that class.

Please also run ./gradlew format && ./gradlew check before committing next. It appears there are some formatting issues that those two tasks will help you work through.

When you are done, will you please change your commits to be more descriptive in the following way. For each commit, describe what you've done and include the issue number like so:

Use MockSecurityContextHolderStrategy

Issue gh-14768

For the final commit, please use Closes gh-14768 instead as this integrates with our build system to close the issue automatically.

@gzhao9 gzhao9 marked this pull request as draft September 6, 2024 06:18
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR, @gzhao9. I've left feedback inline.

Additionally, will you please consider whether you can follow the same strategy for all four files when it comes to setting up mocks? It seems that in one you have a separate class, in another you have a field and a setup method, and in others you use a local method. It would be nice if the code can be more consistent.

Finally, will you please ensure that your commits are in the following format:

Factor out common SecurityContextHolderStrategy mocking

or similar, depending on the work done for each file.

@jzheaux jzheaux added in: core An issue in spring-security-core type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 17, 2024
@jzheaux jzheaux changed the title Refactoring Test Code to Reduce Duplication of Mock Object Creation for Issue#14768 Factor out come common object mocking in tests Sep 17, 2024
@jzheaux jzheaux changed the title Factor out come common object mocking in tests Factor out some common object mocking in tests Sep 17, 2024
@gzhao9 gzhao9 marked this pull request as ready for review September 23, 2024 20:40
@gzhao9
Copy link
Author

gzhao9 commented Sep 23, 2024

Thanks for the cleanup, @gzhao9. I think there is already a MockSecurityContextHolderStrategy available in spring-security-core. Can you please enhance that instead?

I think it would be reasonable to add the default constructor and one that takes a SecurityContext to that class.

Please also run ./gradlew format && ./gradlew check before committing next. It appears there are some formatting issues that those two tasks will help you work through.

When you are done, will you please change your commits to be more descriptive in the following way. For each commit, describe what you've done and include the issue number like so:

Use MockSecurityContextHolderStrategy

Issue gh-14768

For the final commit, please use Closes gh-14768 instead as this integrates with our build system to close the issue automatically.

I found MockSecurityContextHolderStrategy in spring-security-core. However, since they are not in the same package, I would need to change the dependency if I want to reference MockSecurityContextHolderStrategy in org.springframework.security.authorization.method. I believe adding new dependencies should be considered carefully. Therefore, I only added a private method in each test class.

@gzhao9 gzhao9 marked this pull request as draft September 24, 2024 14:35
@gzhao9 gzhao9 marked this pull request as ready for review September 24, 2024 14:41
@gzhao9
Copy link
Author

gzhao9 commented Sep 24, 2024

I first focus on each file code modification. I will rebase commits when there is no need to modify the code or format.

@gzhao9 gzhao9 requested a review from jzheaux September 24, 2024 21:31
@jzheaux
Copy link
Contributor

jzheaux commented Sep 26, 2024

Thanks for the updates, @gzhao9! I think there are two other items from the review that haven't been addressed yet. Are you able to do those as well?

@gzhao9
Copy link
Author

gzhao9 commented Sep 28, 2024

Thanks for the updates, @gzhao9! I think there are two other items from the review that haven't been addressed yet. Are you able to do those as well?

I have modified the code as required.

@jzheaux jzheaux self-assigned this Oct 1, 2024
@jzheaux jzheaux added this to the 6.4.0-RC1 milestone Oct 1, 2024
@jzheaux jzheaux modified the milestones: 6.4.0-RC1, 6.4.x Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core An issue in spring-security-core type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants