Skip to content

Conversation

@nicolad
Copy link
Member

@nicolad nicolad commented Nov 9, 2018

Description

Closes #11535.

This PR is related to this issue: #11535

How has this been tested?

I have checked files that have imported methods from utils.js file can import the same methods from utils folder.

Types of changes

Created new files for methods from utils.js file.

Checklist:

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

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This is a great start, thanks for working on it 👍

I left doc related comments. We didn't do good enough job in the past with documenting this code. It would be great to improve it as we move forward :)

import { addQueryArgs } from '@wordpress/url';
import { visitAdmin } from './';

export async function newPost( {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add JSDoc here? It wasn't provided but it might be very helpful in the long run. At some point we will need to generate docs based on those code comments to make util methods discovery easier.

Copy link
Member Author

@nicolad nicolad Nov 13, 2018

Choose a reason for hiding this comment

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

On next commit I will add JSDoc. Didn't wanted to clutter current commit, although it has quite few changes. But because there was mainly copy-pastying I think it's not that critical to separate into several commits. Correct me if I am wrong.

Copy link
Member

Choose a reason for hiding this comment

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

We squash and merge at the end of the process so it will become a single commit. I'm not concerned that much as long as all e2e tests are green after refactoring is finished :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Great, thank you.

Copy link
Member Author

Choose a reason for hiding this comment

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

JSDoc added here:
568458e

@gziolo gziolo added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Code Quality Issues or PRs that relate to code quality labels Nov 12, 2018
@gziolo gziolo changed the title Extracted some utils methods. Separate e2e test utils functions into their own files Nov 12, 2018
import { pressWithModifier } from './press-with-modifier';
import { META_KEY } from './';

const WP_ADMIN_USER = {
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this logic to its own file and export WP_ADMIN_USER, WP_USERNAME and WP_PASSWORD from there to be able to share in all test helpers?

Copy link
Member Author

Choose a reason for hiding this comment

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

Covered in this commit:
384afa4

*
* @type {string}
*/
export const META_KEY = process.platform === 'darwin' ? 'Meta' : 'Control';
Copy link
Member

Choose a reason for hiding this comment

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

This looks similar to:
https://github.com/WordPress/gutenberg/blob/master/packages/keycodes/src/index.js#L47

I'm wondering if we could use https://github.com/WordPress/gutenberg/blob/master/packages/keycodes/src/index.js#L61
instead or something similar to avoid logic duplication.

I bet it would be easier to perform it in the follow-up PR. For this one, can we consolidate those modifiers in one file? I see that ACCESS_MODIFIER_KEYS is also in their own file. It would read easier if it was colocated with pressWithModifier function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, placing this into follow-up PR is a good idea. Will use your hints when I will create that.
For now I've colocated them here:
90dec9f

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This is awesome. Thanks for immensely improving the way code is structured.

I left some comments on what could be tackled in the follow-up PR.

I'm not sure why e2e tests failed in the last commit. We need to ensure they are green before we merge.

@@ -0,0 +1,6 @@
export async function convertBlock( name ) {
Copy link
Member

Choose a reason for hiding this comment

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

JSDoc is missing.

*/
import { toggleOption } from './toggle-option';

export async function disablePrePublishChecks() {
Copy link
Member

Choose a reason for hiding this comment

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

JSDoc is missing.

It would make sense to consolidate disablePrePublishChecks and enablePrePublishChecks in one file.

*/
import { toggleOption } from './toggle-option';

export async function enablePrePublishChecks() {
Copy link
Member

Choose a reason for hiding this comment

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

The same applies as fo disablePrePublishChecks.


import { WP_BASE_URL } from './config';

export function getUrl( WPPath, query = '' ) {
Copy link
Member

Choose a reason for hiding this comment

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

JSDoc is missing.

*/
import { join } from 'path';

import { WP_BASE_URL } from './config';
Copy link
Member

Choose a reason for hiding this comment

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

Internal dependencies comment is missing.

*/
import { getUrl } from './get-url';

export function isWPPath( WPPath, query = '' ) {
Copy link
Member

Choose a reason for hiding this comment

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

JSDoc is missing.

import { META_KEY } from './';
import { WP_USERNAME, WP_PASSWORD } from './config';

export async function login( username = WP_USERNAME, password = WP_PASSWORD ) {
Copy link
Member

Choose a reason for hiding this comment

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

JSDoc is missing.

*/
import { waitForPageDimensions } from './wait-for-page-dimensions';

export async function setViewport( type ) {
Copy link
Member

Choose a reason for hiding this comment

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

JSDoc is missing.

@@ -0,0 +1,9 @@
export async function switchToEditor( mode ) {
Copy link
Member

Choose a reason for hiding this comment

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

JSDOc is missing

@gziolo gziolo added this to the 4.4 milestone Nov 14, 2018
@gziolo gziolo merged commit 3834138 into WordPress:master Nov 14, 2018
@gziolo
Copy link
Member

gziolo commented Nov 14, 2018

@vadimnicolai - do you plan to open a follow-up PR and improve this further?

@nicolad
Copy link
Member Author

nicolad commented Nov 14, 2018

@vadimnicolai - do you plan to open a follow-up PR and improve this further?

@gziolo, here is the follow-up PR: #11855

@nicolad nicolad deleted the code-quality/11535-extract-test-utilities branch November 14, 2018 13:02
@nicolad nicolad self-assigned this Sep 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Code Quality Issues or PRs that relate to code quality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Separate test utils functions into their own files

2 participants