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.

Sign up for GitHub

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

Jump to bottom

Clever OAuth sign-up/sign-in integration tests #25074

Merged
merged 7 commits into from
Sep 27, 2018
Merged

Clever OAuth sign-up/sign-in integration tests #25074

merged 7 commits into from
Sep 27, 2018

Conversation

islemaster
Copy link
Contributor

Consider this a follow-on to #25062. Adds matching tests for Clever, and does significant cleanup on the Google tests as well, for readability.

New test cases:

  • Clever student sign-up
  • Clever teacher sign-up
  • Clever student sign-up (new flow)
  • Clever teacher sign-up (new flow)
  • Clever student sign-in
  • Clever teacher sign-in

@islemaster
Copy link
Contributor Author

Any thoughts on where to put a helpers file to share helper methods between these two tests?

@islemaster
Copy link
Contributor Author

Looking through coverage reports from the Clever integration tests and these do a pretty good job of hitting all possible branches. Things I've noticed are missed:

We're missing the second half of RegistrationsController#new, but we've already discussed this bug:
image

The GDPR data transfer agreement field isn't covered.
image

The Clever email-already-taken path, and the (maybe unreachable in practice?) old finish registration path where the details we get from Clever aren't sufficient to create the user:
image

Any and all takeover code (Clever teacher forced takeover, Google silent takeover, etc).

Clever admin sign-ups:
image

maddiedierker
maddiedierker reviewed Sep 27, 2018
View reviewed changes
dashboard/test/controllers/omniauth_callbacks_controller/google_oauth2_test.rb
# The user signs in through Google
# The oauth endpoint (which is mocked) redirects to the oauth callback,
# which in turn does some work and redirects to something else: homepage, finish_sign_up, etc.
def sign_in_through_google
Copy link
Contributor

Choose a reason for hiding this comment

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

love this cleanup!

@maddiedierker
Copy link
Contributor

re: where to put a helper file for these tests -- i think either dashboard/test/helpers/ or dashboard/test/controllers/omniauth_callbacks_controller/ would work (slight preference for the former)

maddiedierker
maddiedierker approved these changes Sep 27, 2018
View reviewed changes
Copy link
Contributor

@maddiedierker maddiedierker left a comment

Choose a reason for hiding this comment

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

🥇 yay, tests! great clean-up, too.

@islemaster
Copy link
Contributor Author

Hmm... dashboard/test/helpers seems to contain tests for dashboard/app/helpers, not test-helper code. Maybe I'll ask the group.

@islemaster islemaster merged commit 542ea80 into staging Sep 27, 2018
@islemaster islemaster deleted the clever-sign-up-tests branch September 27, 2018 22:29
This was referenced Sep 27, 2018
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maddiedierker maddiedierker maddiedierker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@islemaster @maddiedierker