-
Notifications
You must be signed in to change notification settings - Fork 231
Fix a pending test #387
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
Fix a pending test #387
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.
Pull Request Overview
This PR addresses a failing OAuth controller spec by ensuring original_callback_url
is reset between examples, updates the test to use a full callback URL (removing the pending
flag and adjusting expectations), and logs the change in the changelog.
- Clear
original_callback_url
in each spec’safter
hook to prevent state leakage - Remove
pending: true
, update thecallback_url
setup, and adjust the expected redirect in the spec - Add a changelog entry for the pending test fix
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
spec/controllers/controller_oauth_spec.rb | Reset state and refine the OAuth login_at redirect spec |
CHANGELOG.md | Add entry for the pending test fix |
Comments suppressed due to low confidence (1)
spec/controllers/controller_oauth_spec.rb:82
- [nitpick] Add a brief comment explaining why
original_callback_url
must be reset here to clarify the need for this cleanup and prevent future confusion.
after do
@@ -81,20 +81,22 @@ def response.body | |||
end | |||
after do | |||
sorcery_controller_external_property_set(:twitter, :callback_url, 'http://blabla.com') | |||
sorcery_controller_external_property_set(:twitter, :original_callback_url, nil) | |||
end | |||
end | |||
|
|||
context 'when callback_url begin with http://' do |
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.
The context description has a grammar error: change "begin" to "begins" to read: context 'when callback_url begins with http://' do
.
context 'when callback_url begin with http://' do | |
context 'when callback_url begins with http://' do |
Copilot uses AI. Check for mistakes.
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.
One little nit-pick here: line 83 has "http://blabla.com". Can we change this to "http://example.com" since blabla.com is an actual website registered in France? example.com will never actually lead anywhere since xample.com, example.net, example.org, and example.edu are all reserved by IANA for testing purposes.
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.
@Beartech Good point — I was also concerned about that. However, domains like blabla.com, bla.com, and test.com are used in multiple files, so I think it’s better to fix them all together in a separate PR rather than in this one. I’ll create that PR as soon as this one is merged.
after do | ||
sorcery_controller_external_property_set(:twitter, :callback_url, 'http://blabla.com') | ||
sorcery_controller_external_property_set(:twitter, :original_callback_url, nil) | ||
end |
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.
This reset is duplicated in both after
hooks. Consider moving the cleanup of callback_url
and original_callback_url
into a shared after(:each)
or an around
block at the top of the spec to DRY up the code.
after do | |
sorcery_controller_external_property_set(:twitter, :callback_url, 'http://blabla.com') | |
sorcery_controller_external_property_set(:twitter, :original_callback_url, nil) | |
end |
Copilot uses AI. Check for mistakes.
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.
Looks good.
Oh. After resolving the conflict following the approval, I can't merge. |
I prefer to stop the current method of updating the Changelog, as it tends to cause conflicts easily. Now that GitHub’s Releases page can automatically generate a Changelog, the importance of maintaining a separate Changelog file has decreased. |
Simply removing pending and adjusting the value allowed the test to pass when run individually(e.g., `rspec ./spec/controllers/controller_oauth_spec.rb:93`), but it failed when runnning the whole file (e.g., rspec ./spec/controllers/controller_oauth_spec.rb) The cause was that the test above was updating `original_callback_url`, and it wasn't beging reset for test next test ( [related code](https://github.com/Sorcery/sorcery/blob/24463e026609072ee51be30a8539a22f97844078/lib/sorcery/controller/submodules/external.rb#L106) ).Resetting `original_callback_url` in each test made the test pass.
fc729eb
to
7052d0b
Compare
Since I haven’t changed anything except resolving conflicts after @Beartech's review, I’ll go ahead and merge this as is. |
Simply removing pending and adjusting the value allowed the test to pass when run individually(e.g.,
rspec ./spec/controllers/controller_oauth_spec.rb:93
), but it failed when runnning the whole file (e.g., rspec ./spec/controllers/controller_oauth_spec.rb)The cause was that the test above was updating
original_callback_url
, and it wasn't beging reset for test next test ( related code ).Resetting
original_callback_url
in each test made the test pass.