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

Keycodes package modifiers usage and adding missing JSDocs. #11855

Merged
merged 13 commits into from Nov 17, 2018

Conversation

Projects
None yet
3 participants
@nicolad
Contributor

nicolad commented Nov 14, 2018

Description

Replaced pressWithModifier method from e2e test utils with modifiers from keycodes package.
Added missing JSDocs in test utils files.

How has this been tested?

Testing will be done with TravisCI

Types of changes

Enhancements

Checklist:

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

@nicolad nicolad referenced this pull request Nov 14, 2018

Merged

Separate e2e test utils functions into their own files #11701

3 of 4 tasks complete
Merge branch 'master' of github.com:WordPress/gutenberg into code-qua…
…lity/11535-test-utils-jsdocs-and-modifiers-update

@nicolad nicolad force-pushed the nicolad:code-quality/11535-test-utils-jsdocs-and-modifiers-update branch from ed09916 to d194732 Nov 15, 2018

Merge branch 'master' of github.com:WordPress/gutenberg into code-qua…
…lity/11535-test-utils-jsdocs-and-modifiers-update
@nicolad

Hi @gziolo, based on this modifierMapping
const SELECT_WORD_MODIFIER_KEYS = process.platform === 'darwin' ? [ 'Shift', 'Alt' ] : [ 'Shift', 'Control' ];
from this file https://github.com/WordPress/gutenberg/blob/master/test/e2e/specs/links.test.js#L18
I have updated this modifier:
https://github.com/WordPress/gutenberg/blob/master/packages/keycodes/src/index.js#L56
This change caused this test to fail:
https://github.com/WordPress/gutenberg/blob/master/packages/edit-post/src/components/keyboard-shortcut-help-modal/test/index.js
What's your thought on this, should I updated the test or should I change back keycodes package ?

@gziolo gziolo requested review from talldan and iseulde Nov 15, 2018

@gziolo gziolo requested a review from mcsf Nov 15, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 15, 2018

I think @talldan have the most experience with Keyboard shortcuts modal so I would defer it to him.

I see also a lot of failing e2e tests because of:

Unknown key: "Ctrl"

In general, I like the direction where this PR is heading. It would be awesome if we could land those changes as it will allow us to reuse existing utilities rather than duplicate them for all tests.

Show resolved Hide resolved packages/keycodes/src/index.js Outdated
@@ -65,7 +65,7 @@ const modifiers = {
*/
export const rawShortcut = mapValues( modifiers, ( modifier ) => {
return ( character, _isApple = isAppleOS ) => {
return [ ...modifier( _isApple ), character.toLowerCase() ].join( '+' );
return [ ...modifier( _isApple ), character.toLowerCase() ].filter( ( key ) => key !== '' ).join( '+' );

This comment has been minimized.

@gziolo

gziolo Nov 15, 2018

Member

What was the reason that filtering is introduced?

This comment has been minimized.

@nicolad

nicolad Nov 15, 2018

Contributor

This is because when we are passing second argument to pressWithModifier there is no need to pass it to rawShortcut also, because key is used only in here await page.keyboard.press( key )

This comment has been minimized.

@talldan

talldan Nov 15, 2018

Contributor

I think it might be tidier to export the modifiers const from keycodes, and write a function that maps the keys exactly as they're needed within the e2e/utils (instead of using rawShortcut in tests).

That would also allow the possibility of adding extra combinations to the list of modifiers that are only used in tests (e.g. the shift+control word selection modifiers) and the production implementation wouldn't need to be touched.

This comment has been minimized.

@nicolad

nicolad Nov 16, 2018

Contributor

Agree, have adjusted this, also had swapped ctrl, because in tests it was used as Control. Also shiftAlt is overwritten in pressWithModifier to align with tests prior usage.

For some reasons Travis has cached META_KEY and SELECT_WORD_MODIFIER_KEYS, which is causing tests to fail, is there a way to clean cache in Travis ?

@talldan

Hey @vadimnicolai - thanks for the changes!

I've added a couple of comments for things that I think need to be addressed.

Show resolved Hide resolved packages/keycodes/src/index.js Outdated
@@ -65,7 +65,7 @@ const modifiers = {
*/
export const rawShortcut = mapValues( modifiers, ( modifier ) => {
return ( character, _isApple = isAppleOS ) => {
return [ ...modifier( _isApple ), character.toLowerCase() ].join( '+' );
return [ ...modifier( _isApple ), character.toLowerCase() ].filter( ( key ) => key !== '' ).join( '+' );

This comment has been minimized.

@talldan

talldan Nov 15, 2018

Contributor

I think it might be tidier to export the modifiers const from keycodes, and write a function that maps the keys exactly as they're needed within the e2e/utils (instead of using rawShortcut in tests).

That would also allow the possibility of adding extra combinations to the list of modifiers that are only used in tests (e.g. the shift+control word selection modifiers) and the production implementation wouldn't need to be touched.

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 16, 2018

I will try to clear caches on Travis and restart build.

@gziolo

This is looking very good, it still needs some adjustments.

There are some issues to resolve:

[0] [0] /home/travis/build/WordPress/gutenberg/test/e2e/specs/links.test.js
[0] [0] 391:28 error 'META_KEY' is not defined no-undef
[0] [0] 461:28 error 'SELECT_WORD_MODIFIER_KEYS' is not defined no-undef
[0] [0]
[0] [0] /home/travis/build/WordPress/gutenberg/test/e2e/specs/undo.test.js
[0] [0] 25:28 error 'META_KEY' is not defined no-undef
[0] [0] 34:28 error 'META_KEY' is not defined no-undef
[0] [0] 39:28 error 'META_KEY' is not defined no-undef
[0] [0]
[0] [0] ✖ 8 problems (5 errors, 3 warnings)

There are still references to the removed constants:

ReferenceError: META_KEY is not defined
  389 | 
  390 | 		// Press Cmd+K to insert a link
> 391 | 		await pressWithModifier( META_KEY, 'K' );
      | 		                         ^
  392 | 
  393 | 		// Wait for the URL field to auto-focus
  394 | 		await waitForAutoFocus();
  at Object.META_KEY (test/e2e/specs/links.test.js:391:28)
  at tryCatch (node_modules/regenerator-runtime/runtime.js:62:40)
  at Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:288:22)
  at Generator.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:114:21)
  at asyncGeneratorStep (test/e2e/specs/links.test.js:15:103)
  at _next (test/e2e/specs/links.test.js:17:194)

● Links › link popover remains visible after a mouse drag event
ReferenceError: SELECT_WORD_MODIFIER_KEYS is not defined
459 |
460 | // Select some text
> 461 | await pressWithModifier( SELECT_WORD_MODIFIER_KEYS, 'ArrowLeft' );
| ^
462 |
463 | // Click on the Link button
464 | await page.click( 'button[aria-label="Link"]' );
at Object.SELECT_WORD_MODIFIER_KEYS (test/e2e/specs/links.test.js:461:28)
at tryCatch (node_modules/regenerator-runtime/runtime.js:62:40)
at Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:288:22)
at Generator.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:114:21)
at asyncGeneratorStep (test/e2e/specs/links.test.js:15:103)
at _next (test/e2e/specs/links.test.js:17:194)
FAIL test/e2e/specs/undo.test.js (6.534s)
● undo › should undo typing after a pause
ReferenceError: META_KEY is not defined
23 | expect( await getEditedPostContent() ).toMatchSnapshot();
24 |
> 25 | await pressWithModifier( META_KEY, 'z' );
| ^
26 |
27 | expect( await getEditedPostContent() ).toMatchSnapshot();
28 | } );
at Object.META_KEY (test/e2e/specs/undo.test.js:25:28)
at tryCatch (node_modules/regenerator-runtime/runtime.js:62:40)
at Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:288:22)
at Generator.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:114:21)
at asyncGeneratorStep (test/e2e/specs/undo.test.js:9:103)
at _next (test/e2e/specs/undo.test.js:11:194)
● undo › should undo typing after non input change
ReferenceError: META_KEY is not defined
32 |
33 | await page.keyboard.type( 'before keyboard ' );
> 34 | await pressWithModifier( META_KEY, 'b' );
| ^
35 | await page.keyboard.type( 'after keyboard' );
36 |
37 | expect( await getEditedPostContent() ).toMatchSnapshot();
at Object.META_KEY (test/e2e/specs/undo.test.js:34:28)
at tryCatch (node_modules/regenerator-runtime/runtime.js:62:40)
at Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:288:22)
at Generator.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:114:21)
at asyncGeneratorStep (test/e2e/specs/undo.test.js:9:103)
at _next (test/e2e/specs/undo.test.js:11:194)

nicolad added some commits Nov 16, 2018

Merge branch 'master' of github.com:WordPress/gutenberg into code-qua…
…lity/11535-test-utils-jsdocs-and-modifiers-update
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…code-quality/11535-test-utils-jsdocs-and-modifiers-update
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…code-quality/11535-test-utils-jsdocs-and-modifiers-update
@nicolad

This comment has been minimized.

Contributor

nicolad commented Nov 16, 2018

Is the Travis environment the same as the container in which e2e tests are runned locally ?
screen shot 2018-11-16 at 18 30 11

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 17, 2018

It should be very close to what you have locally if you use Docker instance provided with the repository.

FAIL test/e2e/specs/links.test.js (41.536s)

There are 9 tests failing in this suite. Everything else passes. Maybe shiftAlt behaves differently on Travis?

https://github.com/WordPress/gutenberg/pull/11855/files#diff-3d57671ff8af77f3854ff531c06ba975R39

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

This comment has been minimized.

@gziolo

gziolo Nov 17, 2018

Member

I think I found what's causing issues. isAppleOS should be called here. It is passed down as function and that's why shiftAlt is always [ SHIFT, ALT ].

This comment has been minimized.

@gziolo

gziolo Nov 17, 2018

Member

Actually, it should be inside shiftAlt. It's not easy to debug using GitHub UI :)

This comment has been minimized.

@nicolad

nicolad Nov 17, 2018

Contributor

You have an eye of the 🐯

@gziolo gziolo added this to the 4.5 milestone Nov 17, 2018

Feedback was addressed :)

@nicolad

This comment has been minimized.

Contributor

nicolad commented Nov 17, 2018

Is it ok if I will add missing JSDocs in a separate PR ?

@gziolo

gziolo approved these changes Nov 17, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 17, 2018

Yes, sure thing. Another nice refactor, thanks for all your help with making e2e test utils better 🙇

@gziolo gziolo merged commit 3b16e5f into WordPress:master Nov 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nicolad nicolad referenced this pull request Nov 19, 2018

Merged

Added missing JSDocs in e2e test utils. #12071

4 of 4 tasks complete

raquelmsmith added a commit to raquelmsmith/gutenberg that referenced this pull request Nov 20, 2018

gziolo added a commit that referenced this pull request Nov 21, 2018

Add e2e tests for the format API (#11948)
* Add e2e tests for the format API

* Fixes some whitespace issues

* Update format-api.test.js

* Add missing dependencies to the script registration

* Add snapshot to test

* Check for custom button, remove extra lines

* Change modifier keys used

From #11855

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Keycodes package modifiers usage and adding missing JSDocs. (WordPres…
…s#11855)

* Replaced pressWithModifier with rawShortcut.

* Replaced SELECT_WORD_MODIFIER_KEYS, META_KEY and ACCESS_MODIFIER_KEYS with rawShortcuts.

* Replaced key strings with constants.

* Replaced rawShortcut usage. TODO: Apply DRY.

* Adjusted failing tests and overwritten some modifiers.

* Removed META_KEY and SELECT_WORD_MODIFIER_KEYS added after syncing with mster.

* Make sure that the check for OS is executed in e2e tests

* Another try to properly call the check for OS in e2e tests

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Add e2e tests for the format API (WordPress#11948)
* Add e2e tests for the format API

* Fixes some whitespace issues

* Update format-api.test.js

* Add missing dependencies to the script registration

* Add snapshot to test

* Check for custom button, remove extra lines

* Change modifier keys used

From WordPress#11855
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment