Run the E2E tests as a user with the author role #11359

notnownikki · 2018-11-01T13:24:34Z

Description

Adds support for E2E_ROLE env var which allows the e2e tests to run as a user with the author role.

It adds a dummy favicon.ico so that we don't (sometimes) error on the 404 when looking at front end posts, and ignores net::ERR_UNKNOWN_URL_SCHEME console message which we also (sometimes) get when viewing a front end post.

The code that deals with plugins switches to the admin user, and back to the test user after the plugin operations are done.

How has this been tested?

Both e2e tests should pass.

At the moment, they require #11395 because the fail is catching that bug.

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

notnownikki · 2018-11-02T11:33:47Z

test/e2e/specs/change-detection.test.js

@@ -94,7 +94,7 @@ describe( 'Change detection', () => {
 		// Toggle post as sticky (not persisted for autosave).
 		await ensureSidebarOpened();

-		const postStickyToggleButton = ( await page.$x( "//label[contains(text(), 'Stick to the Front Page')]" ) )[ 0 ];
+		const postStickyToggleButton = ( await page.$x( "//label[contains(text(), 'Pending Review')]" ) )[ 0 ];


Authors can mark things needing review, they can't stick things to the front page. The effect in the test is the same.

If I'm reading correctly, the E2E tests can still be run by a non-author, in which case the label would not have the changed effect here. Should this be made more generic? Lookup element by its class? A helper utility which returns the button accounting for role?

As far as I can see, no matter what your user role, clicking "Pending Review" marks the post as dirty, and that's what's getting tested here.

Oh, I think I misunderstood the reason for the change. It’s just changing it to an option which is visible for both admins and authors.

notnownikki · 2018-11-02T11:34:34Z

test/e2e/specs/templates.test.js

@@ -60,12 +62,12 @@ describe( 'templates', () => {
 		const STANDARD_FORMAT_VALUE = '0';

 		async function setPostFormat( format ) {
+			await switchToAdminUser();


Switching user will not actually do anything if we're running as the admin user. We'll only switch if we're running as a different user, because the author role cannot manage plugins.

That's good to know; could you leave a comment explaining that in the code? 😄

notnownikki · 2018-11-02T11:35:39Z

test/e2e/support/setup-test-framework.js

@@ -116,6 +116,12 @@ function observeConsoleLogging() {
 			return;
 		}

+		// Viewing posts on the front end can result in this error, which
+		// has nothing to do with Gutenberg.
+		if ( text.includes( 'net::ERR_UNKNOWN_URL_SCHEME' ) ) {


This seems to be a jQuery thing that happens on the front end, and we sometimes would get hit by it, but sometimes the test assertions would complete quickly enough for it not to make the test fail.

Is there an issue anywhere that tracks that? I don't quite follow.

I'm not sure what causes it yet, it seems to be unrelated to Gutenberg, and happens when you view the published post on the front end. I'll investigate further after this merge.

aduth · 2018-11-02T13:03:54Z

test/e2e/specs/change-detection.test.js

@@ -94,7 +94,7 @@ describe( 'Change detection', () => {
 		// Toggle post as sticky (not persisted for autosave).
 		await ensureSidebarOpened();

-		const postStickyToggleButton = ( await page.$x( "//label[contains(text(), 'Stick to the Front Page')]" ) )[ 0 ];
+		const postStickyToggleButton = ( await page.$x( "//label[contains(text(), 'Pending Review')]" ) )[ 0 ];


If I'm reading correctly, the E2E tests can still be run by a non-author, in which case the label would not have the changed effect here. Should this be made more generic? Lookup element by its class? A helper utility which returns the button accounting for role?

test/e2e/specs/templates.test.js

aduth · 2018-11-02T13:33:58Z

test/e2e/specs/change-detection.test.js

@@ -94,8 +94,8 @@ describe( 'Change detection', () => {
 		// Toggle post as sticky (not persisted for autosave).


We're no longer toggling as sticky.

notnownikki · 2018-11-02T13:52:14Z

Not sure what's going on here, I've merged master but the test still fails for authors when it passes on my local branch.

notnownikki · 2018-11-05T12:09:24Z

So this is all working correctly now, and is catching an actual bug.

To recreate:

Set up a brand new WP install.
Set up an author user.
Create a new post, publish it.
Create a new post and add a "Latest Posts" block.
See the 403 error in the console.

Seems that the Latest Posts block is doing something with category information, and requesting /index.php?rest_route=%2Fwp%2Fv2%2Fcategories&per_page=100&context=edit&_locale=user which an author cannot access.

cc: @youknowriad @danielbachhuber

youknowriad · 2018-11-05T12:28:10Z

@notnownikki This is probably a similar bug to the one fixed in #11395, we should use apiFetch directly in the block.

notnownikki · 2018-11-05T13:59:14Z

@aduth I think I addressed your points here, and this would be good to get in because it caught a real bug.

@youknowriad Change to the Latest Posts block is included here now.

danielbachhuber · 2018-11-05T14:02:07Z

packages/block-library/src/latest-posts/edit.js

 		this.toggleDisplayPostDate = this.toggleDisplayPostDate.bind( this );
 	}

+	componentWillMount() {
+		apiFetch( {
+			path: addQueryArgs( `/wp/v2/categories`, CATEGORIES_LIST_QUERY ),


Doesn't this need to iterate the entire list? What if I have 102 categories?

🤷‍♀️ this is just how it was in the original code, I was just fixing it so that it didn't break for authors

Ok. I'll mention on #10873 and we can resolve with that.

youknowriad · 2018-11-05T14:04:10Z

packages/block-library/src/latest-posts/edit.js

+			path: addQueryArgs( `/wp/v2/categories`, CATEGORIES_LIST_QUERY ),
+		} ).then(
+			( categoriesList ) => {
+				this.setState( { categoriesList } );


What if the block is removed at this point, we should guard against it

Added an abort call when the component unmounts, same as the hierarchical term selector.

youknowriad · 2018-11-05T14:08:27Z

packages/block-library/src/latest-posts/edit.js

@@ -56,6 +56,10 @@ class LatestPostsEdit extends Component {
 		);
 	}

+	componentWillUnmount() {
+		invoke( this.fetchRequest, [ 'abort' ] );


Do we support "abort" in apiFetch requests?

if we don't, then the hierarchical term selector is wrong too.

I don't think we do :). I think we did at some point in apiRequest because we were relying on jQuery but it's not the case anymore AFAIK.

Yup, no abort! Ok, I'll do the this.isStillMounted thing instead. Can we fix the term selector in another PR? I'm keen to get this in with as few changes as possible so authors get tested asap

Feedback addressed

tofumatt

This looks good to me and is a solid improvement.

~~I left a few comments and I'm not sure if other comments are resolved (they aren't marked as such automatically by GitHub, so I've flagged @aduth for another review).~~ Never mind, I think maybe I had this tab left open and missed the feedback being addressed.

I think this is a solid improvement though, cheers!

tofumatt · 2018-11-06T22:55:30Z

.travis.yml

@@ -67,5 +67,9 @@ jobs:
    - stage: test
      env: WP_VERSION=latest
      script:
-        - npm install || exit 1


I presume this is run by Travis and thus not needed?

It's run as part of the e2e tests. It didn't used to be, but now it is.

tofumatt · 2018-11-06T22:56:59Z

bin/run-e2e-tests.sh

@@ -7,5 +7,8 @@ cd "$(dirname "$0")/../"
 # Setup local environement
 ( ./bin/setup-local-env.sh )

-# Run the tests
-npm run test-e2e
+if [ "$E2E_ROLE" = "author" ]; then


Seems like the role we want to use could be passed to the script rather than hardcoding an "author-or-admin" choice, but if you want to do that as a follow-up it's fine with me 😄

(It would also let you avoid the if/else here, and instead just pass the role we want, with "admin" being the default. 🤷‍♂️)

tofumatt · 2018-11-06T22:58:52Z

packages/block-library/src/latest-posts/edit.js

+		this.isStillMounted = true;
+		this.fetchRequest = apiFetch( {
+			path: addQueryArgs( `/wp/v2/categories`, CATEGORIES_LIST_QUERY ),
+		} ).then(


What happens if this request fails? We don't use catch here...

Added a catch which sets the list to an empty list.

tofumatt · 2018-11-06T23:00:46Z

test/e2e/specs/templates.test.js

@@ -60,12 +62,12 @@ describe( 'templates', () => {
 		const STANDARD_FORMAT_VALUE = '0';

 		async function setPostFormat( format ) {
+			await switchToAdminUser();


That's good to know; could you leave a comment explaining that in the code? 😄

tofumatt · 2018-11-06T23:01:23Z

test/e2e/specs/templates.test.js

@@ -93,9 +97,13 @@ describe( 'templates', () => {
 		} );

 		it( 'should not populate new page with default block for format', async () => {
+			// This test always needs to run as the admin user.


Could you explain why it needs to run as the admin user?

tofumatt · 2018-11-06T23:03:09Z

test/e2e/support/setup-test-framework.js

@@ -116,6 +116,12 @@ function observeConsoleLogging() {
 			return;
 		}

+		// Viewing posts on the front end can result in this error, which
+		// has nothing to do with Gutenberg.
+		if ( text.includes( 'net::ERR_UNKNOWN_URL_SCHEME' ) ) {


Is there an issue anywhere that tracks that? I don't quite follow.

notnownikki added the [Status] In Progress Tracking issues with work in progress label Nov 1, 2018

notnownikki commented Nov 2, 2018

View reviewed changes

notnownikki added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. and removed [Status] In Progress Tracking issues with work in progress labels Nov 2, 2018

notnownikki requested review from aduth, youknowriad and tofumatt November 2, 2018 11:36

notnownikki mentioned this pull request Nov 2, 2018

Revert refactoring hierarchical term selector using the data module #11395

Merged

aduth previously requested changes Nov 2, 2018

View reviewed changes

aduth reviewed Nov 2, 2018

View reviewed changes

Run the E2E tests as a user with the author role

bd386ab

notnownikki force-pushed the update/e2e-author-role-test branch from 66cd0f1 to bd386ab Compare November 5, 2018 10:41

Fix latest posts block for authors

7edbe39

danielbachhuber reviewed Nov 5, 2018

View reviewed changes

youknowriad reviewed Nov 5, 2018

View reviewed changes

danielbachhuber mentioned this pull request Nov 5, 2018

Fetch all potential categories, attachments, and tags #10873

Closed

About fetch when unmounting

6e28fe4

youknowriad reviewed Nov 5, 2018

View reviewed changes

notnownikki added 2 commits November 5, 2018 14:22

Don't set state if we're not still mounted

b3cfeb2

Update run-e2e-tests.sh

51f21a9

danielbachhuber added this to the 4.3 milestone Nov 6, 2018

chore: Tweak function docs

88b4bd8

tofumatt approved these changes Nov 6, 2018

View reviewed changes

tofumatt requested review from aduth and removed request for aduth November 6, 2018 23:06

notnownikki added 2 commits November 7, 2018 09:37

If fetching categories fails, default to empty list

536c7b4

Comment update

6f2a49f

notnownikki merged commit d6ba8c2 into master Nov 7, 2018

notnownikki deleted the update/e2e-author-role-test branch November 7, 2018 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run the E2E tests as a user with the author role #11359

Run the E2E tests as a user with the author role #11359

notnownikki commented Nov 1, 2018 •

edited

Loading

notnownikki Nov 2, 2018

aduth Nov 2, 2018

notnownikki Nov 2, 2018

aduth Nov 2, 2018

notnownikki Nov 2, 2018

tofumatt Nov 6, 2018

notnownikki Nov 2, 2018

tofumatt Nov 6, 2018

notnownikki Nov 7, 2018

aduth Nov 2, 2018

aduth Nov 2, 2018

notnownikki commented Nov 2, 2018

notnownikki commented Nov 5, 2018

youknowriad commented Nov 5, 2018

notnownikki commented Nov 5, 2018

danielbachhuber Nov 5, 2018

notnownikki Nov 5, 2018

danielbachhuber Nov 5, 2018

youknowriad Nov 5, 2018

notnownikki Nov 5, 2018

youknowriad Nov 5, 2018

notnownikki Nov 5, 2018

youknowriad Nov 5, 2018

notnownikki Nov 5, 2018

tofumatt left a comment •

edited

Loading

tofumatt Nov 6, 2018

notnownikki Nov 7, 2018

tofumatt Nov 6, 2018

tofumatt Nov 6, 2018

notnownikki Nov 7, 2018

tofumatt Nov 6, 2018

tofumatt Nov 6, 2018

tofumatt Nov 6, 2018

		@@ -94,8 +94,8 @@ describe( 'Change detection', () => {
		// Toggle post as sticky (not persisted for autosave).

Run the E2E tests as a user with the author role #11359

Run the E2E tests as a user with the author role #11359

Conversation

notnownikki commented Nov 1, 2018 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notnownikki commented Nov 2, 2018

notnownikki commented Nov 5, 2018

youknowriad commented Nov 5, 2018

notnownikki commented Nov 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notnownikki commented Nov 1, 2018 •

edited

Loading

tofumatt left a comment •

edited

Loading