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

Register subclasses with OmniAuth.strategies #95

Merged
merged 2 commits into from
Apr 9, 2016
Merged

Register subclasses with OmniAuth.strategies #95

merged 2 commits into from
Apr 9, 2016

Conversation

md5
Copy link
Contributor

@md5 md5 commented Apr 7, 2016

This PR updates OmniAuth::Strategies::SAML to register any subclasses as strategies with OmniAuth. This parallels what's done in the omniauth-oauth2 gem (cf. here).

@@ -222,4 +222,13 @@ def post_xml(xml=:example_response)
it 'implements #on_metadata_path?' do
expect(described_class.new(nil)).to respond_to(:on_metadata_path?)
end

describe 'subclasses' do
let(:subclass) { Class.new(described_class) }
Copy link
Member

Choose a reason for hiding this comment

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

instead of declaring the subclass in a let can we just initialize it within the it block?

it 'registers subclasses in OmniAuth.strategies' do
  subclass = Class.new(described_class) # or described_class.new
  expect(OmniAuth.strategies).to include(subclass)
end

I find this approach easier to read as it defines everything the test needs within the test itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated and amended. I also threw described_class into the include check since omniauth-oauth2 is doing that check and it seems reasonable.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 96.8% when pulling ec6f041 on appropriate:register-subclasses into 7175717 on omniauth:master.

@md5
Copy link
Contributor Author

md5 commented Apr 7, 2016

These jruby-head and ruby-head Travis failures are annoying...

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 96.787% when pulling 1894d39 on appropriate:register-subclasses into 7175717 on omniauth:master.

@suprnova32
Copy link
Member

@md5 I agree. We should move them back to the allowed_failures.

@md5
Copy link
Contributor Author

md5 commented Apr 9, 2016

@supernova32 Updated in f74d983 to allow failures of jruby-head and ruby-head.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 96.787% when pulling f74d983 on appropriate:register-subclasses into 7175717 on omniauth:master.

@bufferoverflow bufferoverflow merged commit 146e469 into omniauth:master Apr 9, 2016
@md5 md5 deleted the register-subclasses branch April 9, 2016 05:50
jiongye pushed a commit to jiongye/omniauth-saml that referenced this pull request Jun 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants