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

Testing: Re-enable and refactor login tests that don't use magic links #11666

Merged
merged 2 commits into from
Apr 15, 2020

Conversation

rachelmcr
Copy link
Member

In #11283 we temporarily disabled all login and signup connected tests.

This PR re-enables the login tests that don't use magic links, since they should not have any issues on their own. It also refactors these tests to make the test steps clearer for each test.

Background: These tests were sometimes failing because the magic link tests rely on an activity test rule to be able to invoke the magic link intent directly, and that intent can't always be launched. (The cause for that is still under investigation.) Because the test rule for that intent is launched before every test in the LoginTests class, it can sometimes cause unrelated tests to fail. This PR keeps that test rule and the magic link login test disabled, so it shouldn't cause any issues for the re-enabled tests.

As we work on the magic link issue, we could consider putting all tests that use magic links into their own class (rather than separating tests based on login and signup exclusively) so the test rule is only applied to tests that need it.

To test:

  • Run the optional connected tests on CI and confirm they pass.
  • You can also run the connected tests locally with ./gradlew :WordPress:connectedVanillaDebugAndroidTest. (Note that local test runs include the WPScreenshotTest, which isn't included in the CI run. That test is currently failing, but it's also failing in develop and the failure is related to mocks, not login. We can fix that it a separate PR.)

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@rachelmcr rachelmcr self-assigned this Apr 15, 2020
@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 15, 2020

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 15, 2020

You can test the changes on this Pull Request by downloading the APK here.

@rachelmcr rachelmcr force-pushed the try/enable-login-signup-tests branch from 47eeeae to 90f0918 Compare April 15, 2020 10:52
@rachelmcr rachelmcr marked this pull request as ready for review April 15, 2020 11:25
@rachelmcr rachelmcr requested a review from oguzkocer April 15, 2020 11:25
Copy link
Contributor

@oguzkocer oguzkocer left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @rachelmcr! :shipit:

@oguzkocer oguzkocer merged commit 8f0e676 into develop Apr 15, 2020
@oguzkocer oguzkocer deleted the try/enable-login-signup-tests branch April 15, 2020 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants