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

Env: Await test result of testPortNumberValidation #21394

Merged
merged 1 commit into from
Apr 6, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 3, 2020

This pull request seeks to correct an ineffective test case in @wordpress/env, where the test case does not await the completion of promised tasks.

There are two pressing issues:

  1. The tests currently log to stdout with an error, despite the test case being considered as passing (example)
  2. Related to this, testPortNumberValidation could be throwing an error and we'd not catch it

This passes:

diff --git a/packages/env/test/config.js b/packages/env/test/config.js
index 1de18dd3df..f77dc488f9 100644
--- a/packages/env/test/config.js
+++ b/packages/env/test/config.js
@@ -413,5 +413,6 @@ async function testPortNumberValidation( portName, value ) {
 			`Invalid .wp-env.json: "${ portName }" must be an integer.`
 		);
 	}
+	throw new Error();
 	jest.clearAllMocks();
 }

The errors being logged are not in-fact errors. I expect the issue is that without awaiting the result of the previous, the subsequent mock will interfere with the results of others.

It's simply enough to await completion.

Alternatives to consider:

  1. Create separate test cases, rather than delegate this to a utility (using it.each)
  2. Find some way to parallelize these test cases, not run serialize. It's not possible currently because each test must mock the same readFile implementation.

Example it.each:

it.each(
    [ 'string', [], {}, false, null ].flatMap( ( value ) => [
        [ 'port', value ],
        [ 'testsPort', value ],
    ] )
)(
    'should throw a validaton error if the ports are not numbers (%s, %s)',
    async ( portName, value ) => {
        expect.assertions( 2 );
        readFile.mockImplementation( () =>
            Promise.resolve( JSON.stringify( { [ portName ]: value } ) )
        );
        try {
            await readConfig( '.wp-env.json' );
        } catch ( error ) {
            expect( error ).toBeInstanceOf( ValidationError );
            expect( error.message ).toContain(
                `Invalid .wp-env.json: "${ portName }" must be an integer.`
            );
        }
    }
);

See also: https://jestjs.io/docs/en/api#testeachtablename-fn-timeout

Testing Instruction:

Ensure unit tests pass, and there is no extra logged output:

npm run test-unit packages/env/test/config.js 

@aduth aduth added [Type] Bug An existing feature does not function as intended [Package] Env /packages/env labels Apr 3, 2020
@aduth aduth requested a review from epiqueras as a code owner April 3, 2020 22:13
@github-actions
Copy link

github-actions bot commented Apr 3, 2020

Size Change: 0 B

Total Size: 889 kB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.4 kB 0 B
build/api-fetch/index.js 3.79 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.03 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 102 kB 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.22 kB 0 B
build/block-library/editor.css 7.22 kB 0 B
build/block-library/index.js 110 kB 0 B
build/block-library/style-rtl.css 7.53 kB 0 B
build/block-library/style.css 7.54 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.5 kB 0 B
build/components/index.js 195 kB 0 B
build/components/style-rtl.css 16.6 kB 0 B
build/components/style.css 16.5 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/core-data/index.js 10.7 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.23 kB 0 B
build/date/index.js 5.36 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.05 kB 0 B
build/edit-navigation/index.js 2.71 kB 0 B
build/edit-navigation/style-rtl.css 239 B 0 B
build/edit-navigation/style.css 241 B 0 B
build/edit-post/index.js 92.9 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 10.1 kB 0 B
build/edit-site/style-rtl.css 5.02 kB 0 B
build/edit-site/style.css 5.02 kB 0 B
build/edit-widgets/index.js 7.18 kB 0 B
build/edit-widgets/style-rtl.css 3.74 kB 0 B
build/edit-widgets/style.css 3.73 kB 0 B
build/editor/editor-styles-rtl.css 423 B 0 B
build/editor/editor-styles.css 426 B 0 B
build/editor/index.js 42.8 kB 0 B
build/editor/style-rtl.css 3.49 kB 0 B
build/editor/style.css 3.49 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 6.95 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.93 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.7 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.84 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.5 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

Thank you for doing this. Coincidentally, I also just fixed this in #21229 after I noticed the issue :)

@noahtallen
Copy link
Member

Also, thank you for recommending it.each! I wasn't aware of that when I wrote those tests. It would have made this much simpler :)

@aduth aduth merged commit d3aeeec into master Apr 6, 2020
@aduth aduth deleted the fix/env-test-logging branch April 6, 2020 12:23
@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Env /packages/env [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants