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

Testing: Reimplement pressKeyWithModifier to emulate Cmd+A in macOS #14243

Merged
merged 3 commits into from Apr 4, 2019

Conversation

@aduth
Copy link
Member

commented Mar 5, 2019

This pull request seeks to improve E2E test login mechanism. The loginUser function currently relies on keyboard interactions to "select all" and replace the contents of the username and password fields. This does not work correctly on macOS in the current version of Puppeteer:

GoogleChrome/puppeteer#1313

I was observing that when switching between users in the execution of E2E tests, it would often prepend the new username to the current login, rather than replace it.

The changes here update the utility function to assign the username and password fields explicitly using DOM APIs in a page.evaluate callback.

@aduth aduth requested a review from swissspidy Mar 5, 2019

@nerrad

nerrad approved these changes Mar 5, 2019

Copy link
Contributor

left a comment

Ya this looks like its an acceptable solution for now especially when testing the login form is not the primary focuses of these tests.

However, if the e2e test suite becomes a standard for use across all of WP (and not just GB), might want to consider just clearing the input using page.evaluate and then use the keyPress to add the value?

@aduth

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

might want to consider just clearing the input using page.evaluate and then use the keyPress to add the value?

By value, do you mean to your previous point of it being something of a secondary focus of the test to ensure the login behavior? I'm a bit on the fence with this, where I think we should try to make utilities as lean and optimized as possible, even if it means bypassing real user interactions. Especially when any guarantee we have that it "works" is only incidental, and should probably be tested in standalone (i.e. a test suite covering login behaviors).

@nerrad

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

By value, do you mean to your previous point of it being something of a secondary focus of the test to ensure the login behavior?

Yes that what I meant.

I'm a bit on the fence with this, where I think we should try to make utilities as lean and optimized as possible, even if it means bypassing real user interactions.

I can agree with this assuming that it's made clear in documentation that any of these utility functions should not be assumed as test coverage for what they do. This could probably just be something added to the documentation for the package itself.

I'm mostly wary about e2e tests getting written for WP proper using the same packages and the utils being considered as accurate for test coverage of the action being performed. I agree that if in the future WP core uses this library for future e2e test coverage of general WP ui/ux it should have an explicit test covering logging in (vs using this util).

@gziolo gziolo added this to the 5.3 (Gutenberg) milestone Mar 6, 2019

@aduth

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2019

Noting there's at least one other occurrence of the pressKeyWithModifier( 'primary', 'a' ), seen in #13899 (#13899 (comment)). We should probably fix this everywhere, perhaps with the hacky-but-consistent solution posed at GoogleChrome/puppeteer#1313 (comment) .

@gziolo

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

Noting there's at least one other occurrence of the pressKeyWithModifier( 'primary', 'a' ), seen in #13899 (#13899 (comment)). We should probably fix this everywhere, perhaps with the hacky-but-consistent solution posed at GoogleChrome/puppeteer#1313 (comment) .

On a different note, should we throw an error when someone tries to use this keyboard shortcut to make developer aware that it's not supported?

@aduth

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2019

On a different note, should we throw an error when someone tries to use this keyboard shortcut to make developer aware that it's not supported?

It'd probably be easy enough to create a custom restrict-syntax rule.

@aduth

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2019

Current status of this one: I think I want to try to solve this in a more general fashion for both the login screen, and the other problem cases.

@aduth aduth force-pushed the update/e2e-login-assign branch from a7c76e0 to 85622de Mar 11, 2019

@aduth aduth requested a review from talldan as a code owner Mar 11, 2019

@aduth

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

The recent force-pushed branch includes revisions for all instances of Cmd+A in the tests.

It's been a long time since I've seen this all-green in my local environment:

image

cc @getdave

// alternative to Cmd+A. The second issuance of the key combination is
// handled internerally by the block editor's KeyboardShortcuts utility
// and is not subject to the same buggy emulation.
await __unstableSelectAll();

This comment has been minimized.

Copy link
@aduth

aduth Mar 11, 2019

Author Member

This one was a bit awkward to test, because it truly exists to ensure that Cmd+A twice has the correct behavior, so I opted to keep as close as possible to the original implementation to avoid using selectAllBlocks.

@gziolo
Copy link
Member

left a comment

There are repeated 2 failing tests on two independent nodes:

FAIL specs/reusable-blocks.test.js (12.006s)
  ● Reusable Blocks › multi-selection reusable block can be converted back to regular blocks

FAIL specs/multi-block-selection.test.js (10.526s)
  ● Multi-block selection › should speak() number of blocks selected with multi-block selection

It seems like it is related to the changes where selectAllBlocks() replaces double calls of using keyboard shortcuts.

@@ -16,7 +16,7 @@ npm install @wordpress/e2e-test-utils --save-dev

### activatePlugin

[src/index.js#L1-L1](src/index.js#L1-L1)
[src/index.js#L2-L2](src/index.js#L2-L2)

This comment has been minimized.

Copy link
@gziolo

gziolo Mar 12, 2019

Member

@nosolosw - this only confirms that we need to find a way to output a line in the source code where all API methods are defined rather than where they are exported. In the majority of cases, we will use index.js file to collect public API methods from files which contain the actual implementation.

// Multiselect via keyboard.
await pressKeyWithModifier( 'primary', 'a' );
await pressKeyWithModifier( 'primary', 'a' );
await selectAllBlocks();

This comment has been minimized.

Copy link
@gziolo

gziolo Mar 12, 2019

Member

Should it be still called twice? It doesn't work properly on Travis.

@@ -210,8 +210,7 @@ describe( 'Reusable Blocks', () => {
await page.keyboard.type( 'Second paragraph' );

// Select all the blocks
await pressKeyWithModifier( 'primary', 'a' );
await pressKeyWithModifier( 'primary', 'a' );
await selectAllBlocks();

This comment has been minimized.

Copy link
@gziolo

gziolo Mar 12, 2019

Member

Should it still be called twice? It doesn't work properly on Travis.

// handled internerally by the block editor's KeyboardShortcuts utility,
// and is not subject to the same buggy browser/OS emulation.
await __unstableSelectAll();
await pressKeyWithModifier( 'primary', 'a' );

This comment has been minimized.

Copy link
@gziolo

gziolo Mar 12, 2019

Member

It seems like on Travis, it only selects a single block.

@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019

@aduth

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

I need to revisit this. I'd had some general thoughts on direction being that we remove __unstableSelectAll and handle it from within pressKeyWithModifier. There's also some more recent revelations that this may not ever be changed in Puppeteer (related).

The thought I'd had on an implementation idea was that we run the key-combination and consider whether the default behavior was prevented and, if it wasn't, run selectall afterward assuming that it would be idempotent with a Ctrl+A press. The problem with this is that it's not immediately apparent that we could detect whether an event emitted through page.keyboard would have its default prevented.

@aduth

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2019

I've rebased and pushed a revised implementation which, while on one-hand is an awful abomination, on the other is at least quite well insulated in the implementation of pressKeyWithModifier.

I was observing some local failures which were not immediately explainable. I expect it may fail in Travis as well. I'll plan to look again in the morning.

@gziolo
Copy link
Member

left a comment

I like the direction where it is heading. I have no idea what happens inside the emulation function but if that solves the issue I'm all for it 😄

const overWrittenModifiers = {
...modifiers,
shiftAlt: ( _isApple ) => _isApple() ? [ SHIFT, ALT ] : [ SHIFT, CTRL ],
};
const mappedModifiers = overWrittenModifiers[ modifier ]( isAppleOS );
const mappedModifiers = overWrittenModifiers[ modifier ]( IS_MAC_OS );

This comment has been minimized.

Copy link
@gziolo

gziolo Mar 29, 2019

Member

TypeError: _isApple is not a function

It looks like this should stay unmodified as it breaks keycodes implementation.

@@ -16,12 +88,15 @@ import { modifiers, SHIFT, ALT, CTRL } from '@wordpress/keycodes';
* @param {string} key Key to press while modifier held.
*/
export async function pressKeyWithModifier( modifier, key ) {
const isAppleOS = () => process.platform === 'darwin';
if ( modifier.toLowerCase() === 'primary' && key.toLowerCase() === 'a' ) {
return await emulateSelectAll();

This comment has been minimized.

Copy link
@gziolo

gziolo Mar 29, 2019

Member

Should this emulation be limited to macOS?

This comment has been minimized.

Copy link
@aduth

aduth Apr 2, 2019

Author Member

Should this emulation be limited to macOS?

I'd thought about it, and while true that it doesn't need to occur outside macOS, I'd thought to avoid divergent implementations. The main downside is that, at least currently, emulateSelectAll has to do its own platform detection to determine Meta vs. Control as modifier, which wouldn't be necessary if only applying the emulation specific to macOS.

@gziolo gziolo added the [Type] Bug label Mar 29, 2019

@aduth aduth changed the title Testing: Assign E2E login credentials explicitly Testing: Reimplement pressKeyWithModifier to emulate Cmd+A in macOS Apr 2, 2019

@aduth aduth referenced this pull request Apr 2, 2019

Closed

Some E2E tests fail on the Docker environment #14732

6 of 6 tasks complete
@aduth

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

Aside from #14243 (comment) , there was another obvious outstanding issue fixed in 6b0bf94: I was using the wrong property names meta and ctrl (corrected to metaKey, ctrlKey). Passes locally now (and demonstrated standalone), fingers crossed for Travis 🤞

@gziolo

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

@ashwin-pc - can you check if this PR solves any of the e2e test failures you observed locally?

@getdave

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

@aduth I was experiencing issues with packages/e2e-tests/specs/block-hierarchy-navigation.test.js.

I've tested this against your branch using the following command:

npm run test-e2e packages/e2e-tests/specs/block-hierarchy-navigation.test.js

I'm sorry to say that the result is a test failure. This is due to the same issue (ie: modifiers not working on MacOS).

Screen Shot 2019-04-03 at 09 45 44

@gziolo gziolo dismissed their stale review Apr 3, 2019

My code comments were addressed

@gziolo
Copy link
Member

left a comment

It works for me (macOS), there is one unrelated code failure but it should be fixed with rebase:

Screen Shot 2019-04-03 at 12 07 18

Without this PR I used to see some test failures related to selection from time to time.

@aduth

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

@getdave Can you make sure to run npm run build before the npm run test-e2e command? If I recall correctly, the build no longer done automatically (Edit: #13420), and since pressKeyWithModifier is part of a built module, the changes wouldn't otherwise be reflected.

I'll probably plan to do a rebase here shortly as well, so worth testing again from the latest copy of the branch at that point.

@aduth aduth force-pushed the update/e2e-login-assign branch from 6b0bf94 to a7e85b3 Apr 3, 2019

@getdave

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@aduth @gziolo You'll be pleased to see the npm build step has fixed things:

Screen Shot 2019-04-04 at 10 53 19

@gziolo

gziolo approved these changes Apr 4, 2019

Copy link
Member

left a comment

Let's 🚢

@gziolo gziolo merged commit 454011e into master Apr 4, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@gziolo gziolo deleted the update/e2e-login-assign branch Apr 4, 2019

@gziolo

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Great work @aduth adding all this magic behind to make it work regardless of limitation of Puppeteer 🎉
Thanks, @getdave for help with testing, and @nerrad for reviews.

@gziolo

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Can you make sure to run npm run build before the npm run test-e2e command? If I recall correctly, the build no longer done automatically (Edit: #13420), and since pressKeyWithModifier is part of a built module, the changes wouldn't otherwise be reflected.

We might consider doing a similar trick we do for unit tests where we always use sources for local packages:
https://github.com/WordPress/gutenberg/blob/master/test/unit/jest.config.js#L12-L14

It won't solve all issues but at least will ensure that test logic always uses the latest code.

@ashwin-pc

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@ashwin-pc - can you check if this PR solves any of the e2e test failures you observed locally?

I'm not sure if this comment is still necessary but yes, the e2e test failures i observed have been resolved with this commit. I believe #14732 can be closed now.

@gziolo

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Awesome @ashwin-pc, thanks for checking on your side 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.