You’ve signed the CLA, jjaffeux. Thank you! This pull request is ready for review.
@davidtaylorhq Will give it another look tomorrow, but I think this is already working well.
Can we move this to before to a
let(:user) above the
it .., I can see we are using another
It will be great to have a description here for this piece of work, just in case someone would like to get a little context of where this all fits, or was looking at this PR to gain some historical context.
Same as above, I think we might use want to
user1 once at the top of this spec.
be_empty instead of
I’m kind of reluctant to do this, I followed the same patterns we have in related tests files for other authenticators
same comment it’s same pattern we have in all other specs of this kind. If we want to change this, let’s change it all in one go after this PR. We could probably refactor it to be kind of agnostic.
In terms of tests, I think there is very little point testing every different provider to such a large extent - we end up just testing ManagedAuthenticator repeatedly. We’re not even testing the omniauth parts of the authenticators, we trust they are tested upstream. However, I agree @jjaffeux that we can tidy this up later in one go. I’ll add it to the todo list on Meta
Looks great to me