[Experiment] Use REST API in e2e tests to build up states #33414
Conversation
kevin940726
added
[Type] Technical Prototype
|
Size Change: -1.22 kB (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
kevin940726
force-pushed
the
try/rest-api-e2e-utils
branch
from
1a977b3
to
02fff5a
Compare
kevin940726
requested review from
talldan,
noisysocks,
tellthemachines,
adamziel,
draganescu,
getdave and
youknowriad
There was a problem hiding this comment.
I think it'll be really needed as tests start to involve much more than just posts and blocks. I know the navigation editor/block tests could definitely use this too.
Might want to gather a few more opinions/reviews before merging, but this looks good to me as a first step 👍
|
Have you tried creating a WP plugin that sets the initial state by executing DB calls instead of using REST API? Well, it could as well run the same REST API calls inside PHP for start. |
This is a good idea, but how would Puppeteer trigger the PHP code? It would have to make a page request (e.g. navigate to |
|
Though not related, changing the implementation could probably fix #33275 🤞. If that's the case then we'll have a better understanding of the underlying issue. |
Hmm, I'm not sure I understand. Are DB calls something like SQL commands? I've considered this, but the DB structure in WP is often very complicated and not recommended to be altered directly. If it's something like calling core's PHP functions then I think that have the same effects of calling the REST API? Some core functions are not exposed to REST API though (activating themes/plugins for instance), then I think it would be a good alternative for them. |
With WP hooks you don't need to navigate to a different page, the plugin can just run the necessary logic when loading the page where you are going to do all the testing. Another difference is that you would load the page with all REST API (or their corresponding WP function wrappers) calls executed so you would only have to run test spec because the initial conditions are already set - no need to do a page reload as well like in
I didn't mean literally SQL commands. Whatever is the most appropriate for your use case. If that is a single WP function to call then it's a win. |
Right, but how would we distinguish between a page load that happens at the beginning of a test versus one that happens mid-test? |
One way I can think of is by checking whether a special flag is set in |
The page reload is needed because the test happens after we |
kevin940726
force-pushed
the
try/rest-api-e2e-utils
branch
from
080271d
to
f947052
Compare
|
I opened #33720 as an experiment of something similar to @gziolo's approach, evaluating WordPress commands directly via REST API. Currently, there's only one use case which is to switch between themes. It feels like an overkill to try to introduce something so powerful in our testing toolchain. I prefer using REST API to reset/build states over calling WP commands, as they are well-documented and match more closely to what users would do. We can even think of them as e2e tests for REST APIs in a way. In dea992c, I also fixed an issue where performance tests were always using the latest I think this PR is the way forward, unless there're major issues I would like to merge it and monitor it in trunk. WDYT? |
kevin940726
force-pushed
the
try/rest-api-e2e-utils
branch
from
dea992c
to
0734309
Compare
I don't think it's what I had in mind, because it's using REST API to send PHP code to run through The alternative I considered was a plugin that is always installed in E2E tests' context and it intercepts a special param in the URL. Based on the values passed in this param, we could execute some predefined commands that have access to the full WordPress API. It would be more useful for resetting the WordPress state before all test runs, think about this code that is handled now through Puppeteer: gutenberg/packages/e2e-tests/config/setup-test-framework.js Lines 238 to 242 in 516819e |
This would work too! But I would like to avoid requiring developers to know PHP while all they want to do is to write e2e tests. REST API is already an officially supported interface that we can use, and the gutenberg/packages/e2e-tests/config/setup-test-framework.js Lines 238 to 242 in 516819e I believe most of them can also be implemented using REST API, and we don't have to deprecate existing test utils to achieve that. Just like |
kevin940726
force-pushed
the
try/rest-api-e2e-utils
branch
from
0734309
to
07d770e
Compare
| } | ||
|
|
||
| await deactivatePlugin( 'gutenberg-test-classic-widgets' ); | ||
| await batch( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We actually can't. I think it might have some race condition going on in those API, I didn't look into why though.
There was a problem hiding this comment.
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.
| await rest( { | ||
| method: 'POST', | ||
| path: '/wp/v2/widgets', | ||
| data: { | ||
| id_base: 'search', | ||
| sidebar: 'sidebar-1', | ||
| instance: defaultSearchInstance, | ||
| }, |
There was a problem hiding this comment.
I feel like over time it'd be nice to see some simple helper like await createWidgets( widgets, sidebarId ), but this can always evolve over time with rest and batch as the foundation.
|
Is this new |
| 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. |
There was a problem hiding this comment.
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 😅.
There was a problem hiding this comment.
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?
| ! spinner.classList.contains( 'is-active' ) | ||
| ); | ||
| // Create a search widget in the first sidebar using the default instance. | ||
| await rest( { |
There was a problem hiding this comment.
The tests seem less readable with all these inline rest calls. I believe you should consider hiding them being some utils as otherwise, it's not really clear what the tests are doing unless you wrote it.
There was a problem hiding this comment.
Yep, it has been brought up here: #33414 (comment).
| // Only need to set them once but before any requests. | ||
| await Promise.all( [ setAPIRootURL, setNonce ] ); | ||
|
|
||
| return await apiFetch( options ); |
There was a problem hiding this comment.
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
);There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
restwith a different user account or as logged out? wp-login.phpset 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
There was a problem hiding this comment.
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.
Description
Use REST API to build up/clear states in e2e tests.
Why
Most of our e2e tests require setting up the states before actually running the tests. The way we currently do is to use puppeteer to manually visit those screens and click those buttons. It works fine but it's also very time-consuming. Before each test runs, we have to visit several screens and wait for them to finish, that's dozens of unnecessary and repetitive HTTP requests. What we should do is to build up the states programmatically to skip these steps. As popularized by Cypress (#ref-1, #ref-2), we can focus on what needs to be tested and stop wasting time on building up the states manually.
Benefits are:
Why REST API
At first,
WP-CLIis considered and seemed like the perfect choice for the job. However, after testing it in some tasks, it seems to be too slow when running some basic tasks. For instance, running a simplelistmethod to get all the saved widgets takes about 0.5 to 1 second to run. In fact, all commands take at least 0.5 to 1 second to run. I'm not sure if this is the limitation of the framework, but seems like the bottleneck here is not solvable at the time.The next option would be to use the REST API. The same task that took WP-CLI 1 second can now be done under 100ms instead. Another benefit of using REST API is that we don't need to install anything before we can use it. We can run those tests on any WordPress instance as long as they have REST API supported.
There are disadvantages too though. Some tasks cannot be done with REST API, such as activating themes and plugins. We might want to use
WP-CLIfor those tasks, or call the internal API with auth cookie instead. I'm still in between of different solutions.How it works
This PR tries to migrate some tasks in the widgets e2e tests to use the REST API, which includes:
We can reuse the
@wordpress/api-fetchpackage that we've already been using in the frontend, steadier learning curve.api-fetchonly supports browser environments though. Fortunately, we can workaround that by patchingwindow.fetchtonode-fetch.Some REST API require authentication too. The solution is to add a
basic-authtest plugin to enable HTTP basic authentication on every REST API endpoint. This test plugin should only be used in test environments.Then, we wrap
apiFetchwith the authentication token in a test utilityrest, which combines both to make the REST requests. Finally, all we left is to implement those requests in our tests. It looks something like this:There's also a
batchutil we can use to send batched requests.Result
customizing-widgets.test.jsediting-widgets.test.jsThe tests are being run locally on macOS. The results seem to have significant deviation between runs though.
How has this been tested?
E2E Tests should pass
Types of changes
New feature
Checklist:
*.native.jsfiles for terms that need renaming or removal).