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

ci: enhance our test environment setup #4291

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

iChenLei
Copy link
Member

@iChenLei iChenLei commented Nov 26, 2024

What does this PR do?

  1. Replaced Puppeteer with Playwright for E2E testing.
  2. Updated the minimum supported Node.js version in CI tov14 due to Playwright requirements, removing tests for v10 and v12. This is an improvement over Puppeteer, which requires a minimum of v16.
  3. Forked and modified mocha-headless-chrome to support Playwright.
  4. Updated pipeline titles for clearer CI test purposes.
  5. Added an allow-failure Node.js v23 test to remind us to fix it in the future.
  6. Implemented dependency logic for CI tests: when a contributor submits a pull request, tests will first run on Ubuntu with the latest LTS version of Node.js. Only if these tests pass will the CI proceed to test historical LTS versions of Node.js and different operating systems.

Checklist:

  • Documentation
  • Added/updated unit tests
  • Code complete
Previous PR desc

What does this PR do?

  1. Forked the mocha-headless-chrome project to support using the latest puppeteer.
  2. The latest puppeteer doesn't support Node.js (10, 12, 14) due to some newer ES syntax (mjs, ??=).
  3. The latest puppeteer doesn't support serial testing on Windows-2022(aka windows-latest).
  4. Tested Node 23, but some deprecated APIs caused the less-plugin-clean-css test to fail.

Conclusion:

Removed tests for Node.js (10, 12, 14) as these versions have reached end-of-life. Used Windows-2019 for Windows support due to known issues. Node 23 tests were not added because of the failing less-plugin-clean-css test.

Checklist:

  • Documentation
  • Added/updated unit tests
  • Code complete

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Nov 26, 2024
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Nov 26, 2024
@iChenLei
Copy link
Member Author

iChenLei commented Nov 26, 2024

ERROR: util.isRegExp is not a function

TypeError: util.isRegExp is not a function
    at merge (/home/runner/work/less.js/less.js/packages/less/node_modules/clean-css/lib/utils/compatibility.js:122:44)
    at Compatibility.toOptions (/home/runner/work/less.js/less.js/packages/less/node_modules/clean-css/lib/utils/compatibility.js:159:10)
    at new CleanCSS (/home/runner/work/less.js/less.js/packages/less/node_modules/clean-css/lib/clean.js:44:61)
    at CleanCSSProcessor.process (/home/runner/work/less.js/less.js/packages/less/node_modules/less-plugin-clean-css/lib/clean-css-processor.js:40:26)

@iChenLei
Copy link
Member Author

Error: Command failed: node test/browser/generator/runner.js
/home/runner/work/less.js/less.js/packages/less/node_modules/puppeteer-core/lib/cjs/puppeteer/util/disposable.js:9
Symbol.dispose ??= Symbol('dispose');

node <= 14 not support ??=

@iChenLei iChenLei changed the title fix: pptr timeout issue ci: enhance our test environment setup Nov 27, 2024
@iChenLei
Copy link
Member Author

@matthew-dean @puckowski any suggestion ?

@puckowski
Copy link
Contributor

Thank you for putting this together.

I am fine with updating clean-css at a later time (current version used by Less.js is fairly old) and adding Node 23 support at a later time. I can help test and contribute a PR for that.

I am also fine with dropping Node versions no longer is maintenance.

Disappointing about Windows-2022. I'll take a look at the Puppeteer repository and see what the plans are, if any, for serial testing.

PR diff seems reasonable to me.

@matthew-dean
Copy link
Member

I don't want to derail this, but in the meantime since using Puppeteer, I think Playwright has gotten much more mature.

@iChenLei Does Puppeteer have the support limitations you list? If not, I think we should switch to that.

@iChenLei
Copy link
Member Author

iChenLei commented Dec 8, 2024

I don't want to derail this, but in the meantime since using Puppeteer, I think Playwright has gotten much more mature.

@iChenLei Does Puppeteer have the support limitations you list? If not, I think we should switch to that.

Ok, let me try playwright

@iChenLei
Copy link
Member Author

iChenLei commented Dec 8, 2024

Run npx playwright install chromium
npx: installed 2 in 1.[4](https://github.com/less/less.js/actions/runs/12219190415/job/34085524282?pr=4291#step:6:5)01s
You are running Node.js 12.22.12.
Playwright requires Node.js 14 or higher. 
Please update your version of Node.js.

@iChenLei
Copy link
Member Author

iChenLei commented Dec 8, 2024

@puckowski @matthew-dean Please review, thanks.

windows-latest supported now, and it is known that the nodejs23 test fails, this is just to remind us to fix it in the future

@iChenLei iChenLei requested a review from puckowski December 8, 2024 10:44
@puckowski
Copy link
Contributor

I will try to review PR sometime today (12/08/24).

Copy link
Contributor

@puckowski puckowski left a comment

Choose a reason for hiding this comment

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

Node 14, 16, 18, 20, and 22 support is great. The dependency logic makes sense to me.

I am glad you got macos-latest and windows-latest to work.

Allowing Node 23 to fail will be a good reminder that we need to work on it.

I left a couple of non-blocking comments (can be addressed later if CI becomes flaky), but overall this PR looks good to me!

function handleConsole(msg) {
const args = msg.args() || [];

Promise.all(args.map(a => a.jsonValue().catch(() => '')))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think omitting more robust error handling for every possible exception in this script is okay because YAGNI (You Aren't Gonna Need It), though I think in think case we should try to expand a little bit on why args failed to map to a JSON value.

Maybe something like:

args.map(a =>
            a.jsonValue().catch(error => {
                console.warn("Failed to retrieve JSON value from argument:", error);
                return '';
            })
        )


return page.addInitScript(initMocha, reporter)
.then(() => page.goto(url))
.then(() => page.waitForFunction(() => window.__mochaResult__, { timeout, polling }))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible __mochaResult__ may not be defined after timeout? May want to improve error handling in that scenario so we have a clear message as to why CI failed.

Maybe something like:

 .then(result => {
                        if (!result) {
                            throw new Error('Mocha results not found after waiting. The tests may not have run correctly.');
                        }
                        // Close browser before resolving result
                        return browser.close().then(() => result);
                    });

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Dec 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants