Add verification step for pending invites section #1134
Conversation
ghost
requested a review
from 
ghost
mentioned this pull request
There was a problem hiding this comment.
Thanks for this PR, Sofia! I have a few specific questions about the code changes, but I'm also seeing these linting failures in the CI run on the PR:
/home/circleci/wp-e2e-tests/lib/pages/invite-people-page.js
69:20 warning Missing space before opening brace space-before-blocks
/home/circleci/wp-e2e-tests/lib/pages/people-page.js
17:38 warning There must be a space inside this paren space-in-parens
17:68 warning There must be a space inside this paren space-in-parens
222:31 warning Missing space before opening brace space-before-blocks
/home/circleci/wp-e2e-tests/specs/wp-invite-users-spec.js
27:8 warning 'SidebarComponent' is defined but never used no-unused-vars
27:70 warning Missing semicolon semi
98:2 warning Mixed spaces and tabs no-mixed-spaces-and-tabs
99:2 warning Mixed spaces and tabs no-mixed-spaces-and-tabs
99:6 warning Expected indentation of 5 tab characters but found 4 indent
Let me know if you have any questions about that!
lib/pages/invite-people-page.js
Outdated
| @@ -63,4 +65,9 @@ export default class InvitePeoplePage extends BaseContainer { | |||
|
|
|||
| return DriverHelper.isElementPresent( self.driver, self.noticeSelector ); | |||
| } | |||
|
|
|||
| backToPeopleMenu(){ | |||
There was a problem hiding this comment.
Do we gain something by adding this method to this page object? Since it uses the sidebar component without modifications, what do you think of calling the sidebar component's selectPeople() method directly in the test spec?
There was a problem hiding this comment.
I added a couple of lines more to the method so it does not break when attempting to return to the sidebar menu on mobiles. I usually store all selectors or specific methods that are unique for a certain page in a page object, just also to improve readability in the spec. Am I not seeing a potential downside?
There was a problem hiding this comment.
With this change it's working for me on mobile as well. 👍
I don't think this method is really necessary, since we have a set of components (along with page objects) that we tend to use directly in the specs. Those components (like the top navbar and the sidebar) and their selectors/methods aren't specific to certain pages. I prefer using them directly (instead of wrapping them in a page-specific method) because it provides clarity in the spec about whether you're interacting with a page-specific element or another component, but this approach also works.
One example of a spec using the sidebar component directly:
https://github.com/Automattic/wp-e2e-tests/blob/master/specs/wp-view-site-spec.js
lib/pages/people-page.js
Outdated
| @@ -14,6 +14,7 @@ export default class PeoplePage extends BaseContainer { | |||
| this.searchResultsLoadingSelector = By.css( '.people-profile.is-placeholder' ); | |||
| this.peopleListItemSelector = By.css( '.people-list-item' ); | |||
| this.successNoticeSelector = By.css( '.people-notices__notice.notice.is-success' ); | |||
| this.pendingInviteSelector = By.css('.section-header__label-text'); | |||
There was a problem hiding this comment.
This is also the class on the "Accepted" label on the People > Invites tab, and I see it for example on the People > Team tab, so it isn't specific to the Invites tab either. Is there another label you can use that's specific to the pending invites section of the page?
There was a problem hiding this comment.
With the latest changes this works for me. 👍 It's your call whether to make any further changes (e.g. the backToPeopleMenu() method we discussed) or keep it as it is, based on what you think will work best both here and thinking ahead to similar scenarios we may run into in the future. :)
rachelmcr
added
[Status] Ready to Merge
[Status] In Progress
and removed
[Status] Needs Review
[Status] Ready to Merge
labels
|
As discussed in Slack, let's make sure this tests for the specific invite we just sent instead of any pending invite. |
There was a problem hiding this comment.
The update you made to the selector for the pending invite looks good (I appreciate how the naming is very clear about what you're testing here) and the test works well on multiple screen sizes. My only concern at this point is to clarify the error message for each Can see pending invite step, and then this should be ok to merge.
specs/wp-invite-users-spec.js
Outdated
| return assert.equal( | ||
| mostRecentPendingInviteEmail, | ||
| newInviteEmailAddress, | ||
| 'The email address prefilled on the accept invite page is not correct' ); |
There was a problem hiding this comment.
I'm not sure how this message is related to this test step; could you update it to point to the exact error that might be encountered here?
…ub.com/Automattic/wp-e2e-tests into Add-test-step-to-verify-pending-invite
|
The updated error message looks good! Go ahead and merge this PR when you're ready. :) |
A quick verification step for the existence of pending invites after sending one and accepting it.