Skip to content

chore: bump 'expect' dev dep from v26 to v27 - #8718

Merged
Max Schmitt (mxschmitt) merged 6 commits into
microsoft:masterfrom
mrienstra:patch-3
Oct 8, 2021
Merged

Max Schmitt (mxschmitt) merged 6 commits into
microsoft:masterfrom
mrienstra:patch-3

Conversation

@mrienstra

@mrienstra Michael Rienstra (mrienstra) commented Sep 5, 2021 •

Copy link
Copy Markdown
Contributor

Motivation: jestjs/jest#11640

This change causes npm test to fail, because of a problem when running npm run check-deps, which runs utils/check_deps.js.

More specifically, the function allowExternalImport / alllowExternalImport (in utils/check_deps.js) -- which was very recently added in #8501 -- is failing, when looking at the usage of the expect dev dep.

I played around a little with node_modules/expect/package.json locally to see what kinds of changes would resolve this.

Looks like removing the "exports" section does the trick (it was added in jestjs/jest#9921), but that's not much of a solution.

Adding:

    "./build/types": "./build/types.d.ts",
    "./build/jasmineUtils": "./build/jasmineUtils.js",
    "./build/matchers": "./build/matchers.js"
    "./build/print": "./build/print.js",

... Also does the job, but is presumably unnecessarily specific.

Adding:

    "./build/types": "./build/types.d.ts",
    "./build/*": "./build/*.js"

... Is probably the right solution.
(Bonus: this would also allow removing this existing line: "./build/utils": "./build/utils.js",)

Opened a PR to make this change: jestjs/jest#11816

Just in case there is a better path forward, opening this draft PR to allow conversation to take place both here and in the jest PR. Max Schmitt (@mxschmitt), Pavel Feldman (@pavelfeldman), thoughts?

Comment thread package.json Outdated
@mrienstra

Copy link
Copy Markdown
Contributor Author

FYI I ran tests locally (with local fix to node_modules/expect/package.json to make utils/check_deps.js happy), and everything passed except:

A) [webkit] › proxy.spec.ts:142:1 › should use socks proxy & [webkit] › proxy.spec.ts:153:1 › should use socks proxy in second page failed often -- but not always -- with:

    page.goto: The network connection was lost.
    =========================== logs ===========================
    navigating to "http://non-existent.com/", waiting until "load"
    ============================================================

      146 |   });
      147 |   const page = await browser.newPage();
    > 148 |   await page.goto('http://non-existent.com');

B) selectors-register.spec.ts:20:1 › should work failed once (Timeout of 30000ms exceeded, for all 3 browsers) when running all tests, but passed during 3 re-runs of npm test ./tests/selectors-register.spec.ts

C) [firefox] › inspector/pause.spec.ts:236:3 › pause should populate log with error in waitForEvent failed once when running all tests, but passed during 3 re-runs of npm test ./tests/inspector/pause.spec.ts

Error was:

    Error: expect(received).toEqual(expected) // deep equality

    - Expected  -  1
    + Received  + 11

      Array [
        "page.pause- XXms",
        "page.waitForEvent(console)",
        "waiting for event \"console\"",
        "error: Timeout while waiting for event \"console\"",
    -   "page.click(button)- XXms",
    +   "page.click(button)",
    +   "waiting for selector \"button\"",
    +   "selector resolved to visible <button>Submit</button>",
    +   "attempting click action",
    +   "waiting for element to be visible, enabled and stable",
    +   "element is visible, enabled and stable",
    +   "scrolling into view if needed",
    +   "done scrolling",
    +   "checking that element receives pointer events at (36.1,19)",
    +   "element does receive pointer events",
    +   "performing click action",
      ]

None of these test failures appear to be related to the changes in this PR.

@mrienstra

Michael Rienstra (mrienstra) commented Sep 5, 2021 •

Copy link
Copy Markdown
Contributor Author

Alternate fix, shown in 7e26ebb: add 'expect/build/types', 'expect/build/jasmineUtils', 'expect/build/print', 'expect/build/matchers' to EXTERNAL_IMPORT_ALLOWLIST.

This doesn't seem like an ideal solution, but it decouples this PR from jestjs/jest#11816, which is nice.

Edit: No dice, see update below.

@mrienstra Michael Rienstra (mrienstra) changed the title Bump 'expect' dev dep from v26 to v27 chore: bump 'expect' dev dep from v26 to v27 Sep 5, 2021
@mrienstra

Michael Rienstra (mrienstra) commented Sep 6, 2021 •

Copy link
Copy Markdown
Contributor Author

Turns out extending EXTERNAL_IMPORT_ALLOWLIST just pushes the problem further down:

npm run test

> playwright-internal@1.15.0-next test
> npm run basetest -- --config=tests/config/default.config.ts


> playwright-internal@1.15.0-next basetest
> cross-env PWTEST_CLI_ALLOW_TEST_COMMAND=1 node ./lib/cli/cli test "--config=tests/config/default.config.ts"

node:internal/modules/cjs/loader:484
      throw e;
      ^

Error [ERR_PACKAGE_PATH_NOT_EXPORTED] [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './build/jasmineUtils' is not defined by "exports" in ~/Sites/playwright/node_modules/expect/package.json
    at new NodeError (node:internal/errors:363:5)
    at throwExportsNotFound (node:internal/modules/esm/resolve:321:9)
    at packageExportsResolve (node:internal/modules/esm/resolve:546:3)
    at resolveExports (node:internal/modules/cjs/loader:478:36)
    at Function.Module._findPath (node:internal/modules/cjs/loader:518:31)
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:927:27)
    at Function.Module._load (node:internal/modules/cjs/loader:774:27)
    at Module.require (node:internal/modules/cjs/loader:1013:19)
    at require (node:internal/modules/cjs/helpers:93:18)
    at Object.<anonymous> (~/Sites/playwright/lib/test/matchers/toEqual.js:8:21)
    at Module._compile (node:internal/modules/cjs/loader:1109:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1138:10)
    at Module.load (node:internal/modules/cjs/loader:989:32)
    at Function.Module._load (node:internal/modules/cjs/loader:829:14)
    at Module.require (node:internal/modules/cjs/loader:1013:19) {
  code: 'ERR_PACKAGE_PATH_NOT_EXPORTED'
}

... So I reverted those changes.

@mrienstra

Copy link
Copy Markdown
Contributor Author

Looking for someone who knows the playwright codebase well to take a quick look at:
jestjs/jest#11816
... And chime in if they have anything important to add to that conversation. Thanks!

Pavel Feldman (@pavelfeldman), Dmitry Gozman (@dgozman), Andrey Lushnikov (@aslushnikov), Yury Semikhatsky (@yury-s), Joel Einbinder (@JoelEinbinder), Max Schmitt (@mxschmitt)
(Sorry if this is redundant, but not sure if @-mentions on repos not "watched" will be seen)

@mxschmitt
Max Schmitt (mxschmitt) marked this pull request as ready for review October 8, 2021 13:59
@mxschmitt
Max Schmitt (mxschmitt) force-pushed the patch-3 branch 2 times, most recently from 079396e to 1be2db9 Compare October 8, 2021 14:41
Comment thread tests/tsconfig.json
"allowSyntheticDefaultImports": true,
},
"include": ["**/*.spec.js", "**/*.ts", "index.d.ts"]
"include": ["**/*.spec.js", "**/*.ts", "index.d.ts"],

@mxschmitt Max Schmitt (mxschmitt) Oct 8, 2021 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

note: had to be changed because otherwise the stable-test-runner dogfood globals conflicted with the ToT test-runner globals (tests/playwright-test/entry). e.g.:

$ tsc -p . -p ./tests/
types/testExpect.d.ts:42:22 - error TS2320: Interface 'Matchers<R>' cannot simultaneously extend types 'Omit<Matchers<R>, OverriddenExpectProperties1>' and 'Omit<Matchers<R>, OverriddenExpectProperties>'.
  Named property 'toThrow' of types 'Omit<Matchers<R>, OverriddenExpectProperties1>' and 'Omit<Matchers<R>, OverriddenExpectProperties>' are not identical.

42     export interface Matchers<R> extends Omit<expect.Matchers<R>, OverriddenExpectProperties1> {
                        ~~~~~~~~

types/testExpect.d.ts:42:22 - error TS2320: Interface 'Matchers<R>' cannot simultaneously extend types 'Omit<Matchers<R>, OverriddenExpectProperties1>' and 'Omit<Matchers<R>, OverriddenExpectProperties>'.
  Named property 'toThrowError' of types 'Omit<Matchers<R>, OverriddenExpectProperties1>' and 'Omit<Matchers<R>, OverriddenExpectProperties>' are not identical.

42     export interface Matchers<R> extends Omit<expect.Matchers<R>, OverriddenExpectProperties1> {
                        ~~~~~~~~


Found 2 errors.

error Command failed with exit code 2.

Comment thread src/test/expect.ts
@mxschmitt

Copy link
Copy Markdown
Contributor

Thank you Michael Rienstra (@mrienstra) for your initiative!

@mxschmitt
Max Schmitt (mxschmitt) merged commit 09250fd into microsoft:master Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants