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

[Experiment] Use REST API in e2e tests to build up states #33414

Merged
merged 11 commits into from
Aug 12, 2021
13 changes: 10 additions & 3 deletions bin/plugin/commands/performance.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,13 +150,19 @@ async function setUpGitBranch( branch, environmentDirectory ) {
*
* @param {string} testSuite Name of the tests set.
* @param {string} performanceTestDirectory Path to the performance tests' clone.
* @param {string} environmentDirectory Path to the environment directory.
*
* @return {Promise<WPPerformanceResults>} Performance results for the branch.
*/
async function runTestSuite( testSuite, performanceTestDirectory ) {
async function runTestSuite(
testSuite,
performanceTestDirectory,
environmentDirectory
) {
await runShellScript(
`npm run test-performance -- packages/e2e-tests/specs/performance/${ testSuite }.test.js`,
performanceTestDirectory
performanceTestDirectory,
{ ENVIRONMENT_DIRECTORY: environmentDirectory }
);
const rawResults = await readJSONFile(
path.join(
Expand Down Expand Up @@ -300,7 +306,8 @@ async function runPerformanceTests( branches, options ) {
log( ' >> Running the test.' );
rawResults[ i ][ branch ] = await runTestSuite(
testSuite,
performanceTestDirectory
performanceTestDirectory,
environmentDirectory
);
log( ' >> Stopping the environment' );
await runShellScript(
Expand Down
6 changes: 5 additions & 1 deletion bin/plugin/lib/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,13 @@ const { log, formats } = require( './logger' );
/**
* Utility to run a child script
*
* @typedef {NodeJS.ProcessEnv} Env
*
* @param {string} script Script to run.
* @param {string=} cwd Working directory.
* @param {Env=} env Additional environment variables to pass to the script.
*/
function runShellScript( script, cwd ) {
function runShellScript( script, cwd, env = {} ) {
return new Promise( ( resolve, reject ) => {
childProcess.exec(
script,
Expand All @@ -30,6 +33,7 @@ function runShellScript( script, cwd ) {
NO_CHECKS: 'true',
PATH: process.env.PATH,
HOME: process.env.HOME,
...env,
},
},
function ( error, _, stderr ) {
Expand Down
24 changes: 24 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions packages/e2e-test-utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@
"module": "build-module/index.js",
"dependencies": {
"@babel/runtime": "^7.13.10",
"@wordpress/api-fetch": "file:../api-fetch",
"@wordpress/keycodes": "file:../keycodes",
"@wordpress/url": "file:../url",
"form-data": "^4.0.0",
"lodash": "^4.17.21",
"node-fetch": "^2.6.0"
},
Expand Down
8 changes: 8 additions & 0 deletions packages/e2e-test-utils/src/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
export {
activatePlugin as __experimentalActivatePlugin,
deactivatePlugin as __experimentalDeactivatePlugin,
} from './plugins';
export { activatePlugin } from './activate-plugin';
export { activateTheme } from './activate-theme';
export { arePrePublishChecksEnabled } from './are-pre-publish-checks-enabled';
Expand Down Expand Up @@ -81,5 +85,9 @@ export { showBlockToolbar } from './show-block-toolbar';
export { openPreviewPage } from './preview';
export { wpDataSelect } from './wp-data-select';
export { deleteAllWidgets } from './widgets';
export {
rest as __experimentalRest,
batch as __experimentalBatch,
} from './rest-api';

export * from './mocks';
54 changes: 54 additions & 0 deletions packages/e2e-test-utils/src/plugins.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/**
* External dependencies
*/
import { kebabCase } from 'lodash';

/**
* Internal dependencies
*/
import { rest } from './rest-api';

const pluginsMapPromise = ( async function getPluginsMap() {
const plugins = await rest( { path: '/wp/v2/plugins' } );
const map = {};
for ( const plugin of plugins ) {
// Ideally, we should be using sanitize_title() in PHP rather than kebabCase(),
// but we don't have the exact port of it in JS.
map[ kebabCase( plugin.name ) ] = plugin.plugin;
}
return map;
} )();

/**
* Activates an installed plugin.
*
* @param {string} slug Plugin slug.
*/
async function activatePlugin( slug ) {
const pluginsMap = await pluginsMapPromise;
const plugin = pluginsMap[ slug ];

await rest( {
method: 'PUT',
path: `/wp/v2/plugins/${ plugin }`,
data: { status: 'active' },
} );
}

/**
* Deactivates an active plugin.
*
* @param {string} slug Plugin slug.
*/
async function deactivatePlugin( slug ) {
const pluginsMap = await pluginsMapPromise;
const plugin = pluginsMap[ slug ];

await rest( {
method: 'PUT',
path: `/wp/v2/plugins/${ plugin }`,
data: { status: 'inactive' },
} );
}

export { activatePlugin, deactivatePlugin };
116 changes: 116 additions & 0 deletions packages/e2e-test-utils/src/rest-api.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
/**
* External dependencies
*/
import fetch from 'node-fetch';
import FormData from 'form-data';

/**
* WordPress dependencies
*/
import apiFetch from '@wordpress/api-fetch';

/**
* Internal dependencies
*/
import { WP_BASE_URL } from './shared/config';
import { createURL } from './create-url';

// `apiFetch` expects `window.fetch` to be available in its default handler.
global.window = global.window || {};
global.window.fetch = fetch;

const setAPIRootURL = ( async () => {
// Discover the API root url using link header.
// See https://developer.wordpress.org/rest-api/using-the-rest-api/discovery/#link-header
const res = await fetch( WP_BASE_URL, { method: 'HEAD' } );
const links = res.headers.get( 'link' );
const restLink = links.match( /<([^>]+)>; rel="https:\/\/api\.w\.org\/"/ );

if ( ! restLink ) {
throw new Error( `Failed to discover REST API endpoint.
Link header: ${ links }` );
}

const [ , rootURL ] = restLink;
apiFetch.use( apiFetch.createRootURLMiddleware( rootURL ) );
} )();

const setNonce = ( async () => {
const formData = new FormData();
formData.append( 'log', 'admin' );
formData.append( 'pwd', 'password' );

// Login to admin using fetch.
const loginResponse = await fetch( createURL( 'wp-login.php' ), {
method: 'POST',
headers: formData.getHeaders(),
body: formData,
redirect: 'manual',
} );

// Retrieve the cookies.
const cookies = loginResponse.headers.get( 'set-cookie' );
const cookie = cookies
.split( ',' )
.map( ( setCookie ) => setCookie.split( ';' )[ 0 ] )
.join( ';' );

apiFetch.nonceEndpoint = createURL(
'wp-admin/admin-ajax.php',
'action=rest-nonce'
);

// Get the initial nonce.
const res = await fetch( apiFetch.nonceEndpoint, {
headers: { cookie },
} );
const nonce = await res.text();

// Register the nonce middleware.
apiFetch.use( apiFetch.createNonceMiddleware( nonce ) );

// For the nonce to work we have to also pass the cookies.
apiFetch.use( function setCookieMiddleware( request, next ) {
return next( {
...request,
headers: {
...request.headers,
cookie,
},
} );
} );
} )();

/**
* Call REST API using `apiFetch` to build and clear test states.
*
* @param {Object} options `apiFetch` options.
* @return {Promise<any>} The response value.
*/
async function rest( options = {} ) {
// Only need to set them once but before any requests.
await Promise.all( [ setAPIRootURL, setNonce ] );

return await apiFetch( options );
Copy link
Member

@gziolo gziolo Aug 12, 2021

Choose a reason for hiding this comment

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

Would it be possible to use wp.apiFetch instead that should have already all params set when the user is logged in. Something like this:

await page.evaluate( ( params ) =>
	wp.apiFetch( params ),
	options
);

Copy link
Member Author

Choose a reason for hiding this comment

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

That would make the test depends on the page it's currently running on. Some pages don't have wp.apiFetch available by default, and sometimes the logged in user doesn't have the necessary permissions. It also means that a reload is required after making such requests. Calling it server-side seems like a more robust approach to me.

Copy link
Member

Choose a reason for hiding this comment

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

I must admit that it's all confusing. In the current shape, the rest method is doing a REST API request as an admin with hardcoded credentials to admin + password. If the goal is to make it general purpose then it's far from it. Some concerns:

  • e2e tests can be executed also with sites that don't have the admin account with the default credentials
  • should it be possible to use rest with a different user account or as logged out?
  • wp-login.php set the cookie so it's very likely that once the test refreshes the page then the rest of the test is going to be performed as an admin

Copy link
Member Author

Choose a reason for hiding this comment

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

rest isn't the best name here. It's running in the nodejs process rather than the browser context, so it's independent to the test and won't affect the current logged in user. As mentioned in the description of the PR, it should only be used to setup/clear states between or during tests. I'll keep searching for a better name here, fortunately it's still being marked as experimental.

}

/**
* Call a set of REST APIs in batch.
* See https://make.wordpress.org/core/2020/11/20/rest-api-batch-framework-in-wordpress-5-6/
* Note that calling GET requests in batch is not supported.
*
* @param {Array<Object>} requests The request objects.
* @return {Promise<any>} The response value.
*/
async function batch( requests ) {
return await rest( {
method: 'POST',
path: '/batch/v1',
data: {
requests,
validation: 'require-all-validate',
},
} );
}

export { rest, batch };
39 changes: 18 additions & 21 deletions packages/e2e-test-utils/src/widgets.js
Original file line number Diff line number Diff line change
@@ -1,32 +1,29 @@
/**
* Internal dependencies
*/
import { activatePlugin } from './activate-plugin';
import { deactivatePlugin } from './deactivate-plugin';
import { visitAdminPage } from './visit-admin-page';
import { rest, batch } from './rest-api';

/**
* Delete all the widgets in the widgets screen.
*/
export async function deleteAllWidgets() {
// TODO: Deleting widgets in the new widgets screen is cumbersome and slow.
// To workaround this for now, we visit the old widgets screen to delete them.
await activatePlugin( 'gutenberg-test-classic-widgets' );
const [ widgets, sidebars ] = await Promise.all( [
rest( { path: '/wp/v2/widgets' } ),
rest( { path: '/wp/v2/sidebars' } ),
] );

await visitAdminPage( 'widgets.php' );
await batch(
widgets.map( ( widget ) => ( {
method: 'DELETE',
path: `/wp/v2/widgets/${ widget.id }?force=true`,
} ) )
);

let widget = await page.$( '.widgets-sortables .widget' );

// We have to do this one-by-one since there might be race condition when deleting multiple widgets at once.
while ( widget ) {
const deleteButton = await widget.$( 'button.widget-control-remove' );
const id = await widget.evaluate( ( node ) => node.id );
await deleteButton.evaluate( ( node ) => node.click() );
// Wait for the widget to be removed from DOM.
await page.waitForSelector( `#${ id }`, { hidden: true } );

widget = await page.$( '.widgets-sortables .widget' );
}

await deactivatePlugin( 'gutenberg-test-classic-widgets' );
await batch(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be batched with the previous batch, or are there concerns about limits? (I think the limit is 25 or something). The limit might actually be a concern here if a test ever adds lots of widgets.

Copy link
Member Author

Choose a reason for hiding this comment

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

We actually can't. I think it might have some race condition going on in those API, I didn't look into why though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok 👍

Might still be worth looking into the batch limit thing (some background - #28709) as a follow-up, I think this batch function should ideally split the requests array based on the limit when it's over the limit and then make sequential requests for each part.

sidebars.map( ( sidebar ) => ( {
method: 'POST',
path: `/wp/v2/sidebars/${ sidebar.id }`,
body: { id: sidebar.id, widgets: [] },
} ) )
);
}
15 changes: 15 additions & 0 deletions packages/e2e-tests/jest.performance.config.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,22 @@
/**
* External dependencies
*/
const path = require( 'path' );

const { ENVIRONMENT_DIRECTORY = '<rootDir>' } = process.env;

module.exports = {
...require( '@wordpress/scripts/config/jest-e2e.config' ),
testMatch: [ '**/performance/*.test.js' ],
setupFiles: [ '<rootDir>/config/gutenberg-phase.js' ],
moduleNameMapper: {
// Use different versions of e2e-test-utils for different environments
// rather than always using the latest.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

To use the PR's version of e2e-test-utils so that changes in that package can be picked up when running the performance tests. This is not needed anymore though since we made the changes opt-in and experimental now. Just forgot to revert it 😅.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm wrong, this was already the case, PRs use the PR's version of the e2e-test-utils always. There's already a --tests-env option in the performance PR?

[ `@wordpress\\/e2e-test-utils$` ]: path.join(
ENVIRONMENT_DIRECTORY,
'packages/e2e-test-utils/src'
),
},
setupFilesAfterEnv: [
'<rootDir>/config/setup-performance-test.js',
'@wordpress/jest-console',
Expand Down
11 changes: 0 additions & 11 deletions packages/e2e-tests/plugins/classic-widgets.php

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
* WordPress dependencies
*/
import {
activatePlugin,
__experimentalActivatePlugin as activatePlugin,
activateTheme,
deactivatePlugin,
__experimentalDeactivatePlugin as deactivatePlugin,
visitAdminPage,
showBlockToolbar,
clickBlockToolbarButton,
Expand Down
Loading