Update config@3.3.7 (from 3.3.3)

[tomalec]

⚠️ this PR is based on #33827 - it does not have to be connected, but it's easier to guide to the actual use- and test case I have.

All Submissions:

• Have you followed the WooCommerce Contributing guideline?
• Does your code follow the WordPress' coding standards?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Update config@3.3.7 (from 3.3.3)

to avoid ReferenceError: node_env_var_name is not defined when external extension imports and transforms the admin-e2e-tests directly from node_modules.

Include node-config/node-config#642

How to test the changes in this Pull Request:

Bug repro

1. Clone an extension that uses it, like https://github.com/woocommerce/google-listings-and-ads/tree/b14771982bb0c84d6252b27f36684114da06b85a
2. run npx wc-e2e docker:up && npx wc-e2e test:e2e

fix

1. Clone an extension that uses it, like https://github.com/woocommerce/google-listings-and-ads/tree/b14771982bb0c84d6252b27f36684114da06b85a
2. Install this version of e2e-*

   cd node_modules/@woocommerce/e2e-environment/
   npm i --save jest-puppeteer@^5.0.4
   npm i --save config@^3.3.7
   cd ../e2e-utils/
   npm i --save config@^3.3.7
   cd ../../../
   

3. run npx wc-e2e docker:up && npx wc-e2e test:e2e

Other information:

I was not able to run build and fully test locally due to #33684

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes, as applicable?
• Have you successfully run tests with your changes locally?
• Have you created a changelog file for each project being changed, ie pnpm changelog add --filter=<project>?

None of the selected packages has a "changelog" script

So I added the changelog entry manually to CHANGELOG.md

FOR PR REVIEWER ONLY:

• I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

[github-actions]

Test Results Summary

Commit SHA: 33325b1

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	1m 10s
E2E Tests	189	0	0	6	0	195	15m 49s

---

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

[Luc45]

This looks like a pretty straightforward change, but e2e tests are all failing on CI in this PR.

I have re-run them just to be sure, and the failures are consistent. I think e2e-environment might be unable to load the config using the updated version for some reason.

[tomalec]

I think e2e-environment might be unable to load the config using the updated version for some reason.

Do you have any hint how to debug it locally?
As mentioned in #33852 I cannot even run build command locally.

[tomalec]

I also guess it may fail due to lack of appPath to resolve config paths, due to #33684

[Luc45]

I also guess it may fail due to lack of appPath to resolve config paths, due to #33684

I don't think so, because other PRs that are up-to-date with trunk are passing: #33905

This seems to have something to do with the config package. I'll take a closer look into it. However, we are in the process of deprecating e2e-environment, so I strongly suggest using the new setup based on Playwright + wp-env.

We are just waiting for this to be merged: #33850

Then you can follow this documentation (feedback on documentation is highly appreciated): https://github.com/woocommerce/woocommerce/blob/db50c3249720711f61f7bae29b8a7b5ffba53ba2/plugins/woocommerce/e2e/README.md

[tomalec]

I don't think so, because other PRs that are up-to-date with trunk are passing: #33905

👌

---

However, we are in the process of deprecating e2e-environment, so I strongly suggest using the new setup based on Playwright + wp-env.

Cool, thanks. That's something new to me.

Then you can follow this documentation (feedback on documentation is highly appreciated): db50c32/plugins/woocommerce/e2e/README.md

This doc seem to explain how to test woocommerce itself. What I'm trying to do is to test my Woo extension with admin-e2e-tests suite. Do you have some guides on that specifically? as its README still suggests the use of e2e-environment

[alopezari]

Hi @tomalec!

First of all, thank you very much for creating this PR.

We might need to update that documentation since the tests exposed at the bottom of the page are already covered by the new setup based on Playwright that @Luc45 mentioned.

If you check the table from the Admin E2E tests documentation, you will see that they are aligned with the activate-and-setup, admin-analytics, admin-marketing, admin-tasks and basic.spec.js Playwright tests.

So yes, you can rely on these Playwright specs to test this PR :)

You can find here more information about how to run these tests. In concrete, you can use this example to run individually the specs mentioned above, which covers the same as the previous Admin E2E tests:

cd plugins/woocommerce && USE_WP_ENV=1 pnpm playwright test --config=e2e/playwright.config.js ./e2e/tests/activate-and-setup/basic-setup.spec.js (running a single test)

Please, let us know if anything is not clear or if you need more information!

[tomalec]

Sorry, @alopezari but I'm still pretty much confused.

So yes, you can rely on these Playwright specs to test this PR :)

What you suggest above is to "test" - check that this PR is acceptable? "test" - check that this PR solves the original issue, which was running locally e2e tests in the extension I develop? or "test" in what sense?

You can find here more information about how to run these tests. In concrete, you can use this example to run individually the specs mentioned above, which covers the same as the previous Admin E2E tests:

Butt this is about testing WooCommerce - the plugin from this repo, and not my use case which is testing WooCommerce extension locally.

[tomalec]

Also, speaking of how to run all of that in this repo. pnpm install fails, as it requires an old version of pnpm to which I cannot downgrade:

$ nvm use && pnpm install

Found '/home/tomalec/repos/woocommerce/.nvmrc' with version <v16>
Now using node v16.14.2 (npm v8.5.0)
 ERR_PNPM_UNSUPPORTED_ENGINE  Unsupported environment (bad pnpm and/or Node.js version)

Your pnpm version is incompatible with "/home/tomalec/repos/woocommerce/plugins/woocommerce".

Expected version: ^6.24.2
Got: 7.5.2

This is happening because the package's manifest has an engines.pnpm field specified.
To fix this issue, install the required pnpm version globally.

To install the latest version of pnpm, run "pnpm i -g pnpm".
To check your pnpm version, run "pnpm -v".

$ pnpm i -g pnpm@6.24.2
Packages: +1
+
Packages are hard linked from the content-addressable store to the virtual store.
  Content-addressable store is at: /home/tomalec/.local/share/pnpm/store/v3
  Virtual store is at:             .pnpm

/home/tomalec/.local/share/pnpm/global/5:
+ pnpm 6.24.2 (7.9.0 is available)

Progress: resolved 1, reused 0, downloaded 1, added 1, done

$ pnpm i
 ERR_PNPM_UNSUPPORTED_ENGINE  Unsupported environment (bad pnpm and/or Node.js version)

Your pnpm version is incompatible with "/home/tomalec/repos/woocommerce/plugins/woocommerce".

Expected version: ^6.24.2
Got: 7.5.2

This is happening because the package's manifest has an engines.pnpm field specified.
To fix this issue, install the required pnpm version globally.

To install the latest version of pnpm, run "pnpm i -g pnpm".
To check your pnpm version, run "pnpm -v".

[alopezari]

What you suggest above is to "test" - check that this PR is acceptable? "test" - check that this PR solves the original issue, which was running locally e2e tests in the extension I develop? or "test" in what sense?
Butt this is about testing WooCommerce - the plugin from this repo, and not my use case which is testing WooCommerce extension locally.

Sorry @tomalec, I missed the extension part and thought you wanted to run regression tests to check the config change didn't cause any major inconveniences. Sorry for the confusion.

To be honest, I don't have a clear answer now about how to proceed in your case (this applies also to the PNPM version problem), since we're in the middle of a migration and some things are changing. But I will help you unblock this PR considering it's been here for a while. I'll look for the answers and come back to you once I get something.

[tomalec]

Through my manual cop'n'pasting & checks mentioned in OP, I managed to test it solves at least one step with setting up admin e2e tests for external extension.

Speaking of running local woocommerce/woocommerce builds and checks, I still struggle to run pnpm run build. I managed to downgrade my pnpm to 6.24, but what I got is:

 ~/repos/woocommerce   trunk  nvm use
Found '/home/tomalec/repos/woocommerce/.nvmrc' with version <v16>
Now using node v16.14.2 (npm v8.5.0)
  ~/repos/woocommerce   trunk  pnpm --version
6.24.2
  ~/repos/woocommerce   trunk  pnpm run build

> woocommerce-monorepo@ build /home/tomalec/repos/woocommerce
> pnpm exec turbo run turbo:build -- -- 

 ERROR  workspace configuration error: package.json: no workspaces found. Turborepo requires npm workspaces to be defined in the root package.json
 ELIFECYCLE  Command failed with exit code 1.

[zhongruige]

Hi @tomalec, thanks for getting this in! Looks like we have a merge conflict on pnpm-lock.yaml that we'll need to resolve before we can merge in these changes.

[rodelgc]

@tomalec once the conflicts are resolved, feel free to ping me for a re-review so we could merge it in. Thanks!

[tomalec]

@rodelgc I redid the changes as the conflicts were too far behind. I also added config to syncpack to keep it on track in the future

[alopezari]

Hi @tomalec!

The changes look good to me.

I followed the testing instructions with the google-listings-and-ads plugin and the tests failed always with the same failure:

At setup page › Merchant who has their Jetpack connected, but Google not yet connected › after clicking the "Connect your Google account" button › should sent an API request to connect Google account, and redirect to the returned URL

    expect(received).resolves.toMatch(expected)

    Expected substring: "Log In"
    Received string:    "WordPress › Installation"

However, I don't think it's related to the config change since these tests also fail for that git reference (b14771982bb0c84d6252b27f36684114da06b85a) without updating to config@3.3.7 (in other words, without the changes from this PR). It looks good to me since there isn't any execution error.

The config version was also updated in the WooCommerce package.json, and the tests passed in this PR, so that looks good to me too.

However, considering the WooCommerce package.json has been updated, I'd like to add @woocommerce/atlas to the review to double check, and also because they are the Monorepo owners.

Apart from the review from Atlas, there are new conflicts in the pnpm-lock.yml. Once it is solved and Atlas approves the review, we're happy to merge this 👍 Thank you for your work and patience!

[tomalec]

I rebased and force-pushed changes to solve pnpm conflicts.

I followed the testing instructions with the google-listings-and-ads plugin and the tests failed always with the same failure:

That branch was made over 6months ago, I guess the entire testing environment changed a lot. Given it was not implemented on a stable foundation back then, and there was still not a closed set of problems with importing admin-e2e-tests, I don't expect it to run 100% successfully today.

[samueljseay]

In syncpack the filter attribute means that syncpack will ignore any dependencies that don't match the filter even if they have a version group object like this.

The API is sort of counterintuitive for what we want to achieve right now and there are possibly some alternatives, but for now you'll need to add "config" to the filter regex if you want this to work.

[tomalec]

Thanks, I missed it.

Addressed in 105d95a

The API is sort of counterintuitive for what we want to achieve right now

On the other way around: API is intuitive, but our usage is against the design. AFAIU, syncpack by default, tries to keep all packages in sync, then expose versionGroups to group the exception rules.

What we do is, ignore everything and sync only a few. This is probably the only way to start cleaning it up gradually, without "do it all" revolution.

[tomalec]

@samueljseay I resolved conflicts and the PR is ready for another look :)

[samueljseay]

Looking good 🚀

[nigeljamesstevenson]

Hi folks, I am Porter this week and it appears this one has slipped through the net with the Woo DM last week...

Unfortunately, there are some conflicts:

• .syncpackrc
• pnpm-lock.yaml

Feel free to resolve and I will merge - @samueljseay has already stated that he is happy with this one - am sure @tomalec would like to get this one done and dusted 😊

[tomalec]

Hi @nigeljamesstevenson, I resolved the conflicts again. Could you approve and merge?

[codecov]

Codecov Report

Merging #33828 (33325b1) into trunk (b717ce9) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff            @@
##             trunk   #33828   +/-   ##
========================================
  Coverage     46.6%    46.6%           
  Complexity   17164    17164           
========================================
  Files          429      429           
  Lines        64724    64724           
========================================
  Hits         30193    30193           
  Misses       34531    34531           

Conflicts were resolved

[samueljseay]

🚀 Thank you so much for this, love seeing everyone help us move forward with syncpack.