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

tests(starter): add tests for oauth2 and identity provider #4815

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

joaquinfelici
Copy link
Contributor

@joaquinfelici joaquinfelici commented Nov 26, 2024

related to #4567

@joaquinfelici joaquinfelici self-assigned this Nov 26, 2024
@joaquinfelici joaquinfelici force-pushed the 4567-Oauth2Tests branch 2 times, most recently from 32ee57e to 654fe40 Compare November 26, 2024 11:42
@joaquinfelici joaquinfelici changed the title feat(run): add oauth2 identity provider feat(run): add tests for oauth2 authentication Nov 26, 2024
@joaquinfelici joaquinfelici added the ci:spring-boot Runs the integration tests for the Spring Boot starter. label Dec 5, 2024
@joaquinfelici joaquinfelici force-pushed the 4567-Oauth2Tests branch 3 times, most recently from 6630ba8 to 2e0cf6b Compare December 9, 2024 10:19
@joaquinfelici joaquinfelici marked this pull request as ready for review December 9, 2024 13:19
@joaquinfelici joaquinfelici changed the title feat(run): add tests for oauth2 authentication feat(spring-boot): add tests for oauth2 authentication and identity provider Dec 9, 2024
@psavidis
Copy link
Contributor

Hey @joaquinfelici,

Here is my review:

  • Questions: I've got one question
  • Observations: Verified the tests that rely on the existence of beans (ref1, ref)
  • Tests: According to the Testing Best Practices, the tests should follow a shouldReturnSomething naming convention. Instead of commenting on each test, i've added one example which could have also been more helpful to me to understand the testing logic.

My next step recommendation: Let's pair up to add a few comments on the when / then sections, adapt the names and answer questions.

@psavidis
Copy link
Contributor

Hey @joaquinfelici,

Here is my review:

  • Questions: I've got one question
  • Observations: Verified the tests that rely on the existence of beans (ref1, ref)
  • Tests: According to the Testing Best Practices, the tests should follow a shouldReturnSomething naming convention. Instead of commenting on each test, i've added one example which could have also been more helpful to me to understand the testing logic.

My next step recommendation: Let's pair up to add a few comments on the when / then sections, adapt the names and answer questions.

  • We agreed with @joaquinfelici that the test naming conventions can diverge from the best practices guide as they are whitebox tests, tied to spring security and the attempt to rename them made the result worse.
  • Some comments will be added to the given-when-then blocks to add context
  • After that, the review is concluded and is approved.

@joaquinfelici joaquinfelici changed the title feat(spring-boot): add tests for oauth2 authentication and identity provider tests(starter): add tests for oauth2 and identity provider Dec 12, 2024
@joaquinfelici joaquinfelici merged commit 2d02d61 into master Dec 12, 2024
5 checks passed
@joaquinfelici joaquinfelici deleted the 4567-Oauth2Tests branch December 12, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:spring-boot Runs the integration tests for the Spring Boot starter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants