Fix logout for connected tests and re-enable Slack notifications #11850
Conversation
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can test the changes on this Pull Request by downloading the APK here. |
| @@ -33,7 +35,7 @@ public MePage go() { | |||
| // Using the settings button as a marker for successfully navigating to the page | |||
| while (!isElementDisplayed(appSettings)) { | |||
| clickOn(R.id.nav_sites); | |||
| clickOn(R.id.me_item); | |||
| clickOn(onView(allOf(withId(R.id.me_item), withEffectiveVisibility(Visibility.VISIBLE)))); | |||
There was a problem hiding this comment.
Given that clickOn is our own custom click action, I don't love modifying the element we're passing it here. It feels cleaner to update the clickOn method itself to better handle scenarios like this. What do you think?
There was a problem hiding this comment.
Let's give it a shot.
There was a problem hiding this comment.
@rachelmcr I've changed the onView(withId($elementId)) instances with a helper that also checks for visibility. This covers most of the matchers, but there are still some that checks for text or a specific child. I am not sure if we are doing the right thing by having the visibility matcher everywhere. That probably is due to my unfamiliarity with Espresso, so if you think we should change those as well, I can.
There was a problem hiding this comment.
I am not sure if we are doing the right thing by having the visibility matcher everywhere.
Agreed. That helper feels off especially when combining withEffectiveVisibility and isDisplayed when it's used in various methods. I don't have as much experience with Espresso but my understanding is that those matchers are meant to be alternatives to each other, not additive checks (as explained in the withEffectiveVisibility documentation.
I'm going to backtrack and suggest that we use your first approach, to limit how much we're using withEffectiveVisibility (only using it with elements that strictly require it). Sorry for going back and forth on that.
I'm also making a note to take another look at this behavior when I have time, to see if there's some other solution that I'm missing.
There was a problem hiding this comment.
@rachelmcr That's fine by me, but do you mind if we merge this PR and then change it back in a subsequent PR so the tests would work again? I don't have time to follow up with this today and might not be able to do it tomorrow either, so I don't want to postpone fixing the tests for too long.
There was a problem hiding this comment.
Yes, I just went back and approved the PR before seeing this comment. I'll go ahead and merge it for now. 👍
There was a problem hiding this comment.
As I mentioned in my comment, I think it might be better to go back to a simpler fix that doesn't add all the checks for visibility with the helper method. (Now that I see it, I think that wasn't the right recommendation on my part.) That said, I'm ok with merging this fix as-is to fix the tests and then taking more time to consider the approach we use here.
I'm not going to merge the PR for now, in case you want to make changes, but please feel free to merge it if you feel comfortable with it as is.
This PR adds an extra visibility matcher to
me_itemmenu button in the "My Site" screen so the tests can logout. I couldn't figure out what caused the matcher to stop working since there doesn't seem to be a change to theMySiteFragmentwhen the tests started to fail. Regardless, this matcher change is working locally, so hopefully it'll fix the ones in Firebase as well.I've also re-enabled to Slack notifications since the tests were consistently passing before this issue, so assuming this PR fixes the issue, I don't think we need to keep that disabled anymore.
To test:
PR submission checklist:
RELEASE-NOTES.txtif necessary.