Skip to content

Conversation

@matthew-dean
Copy link
Member

Why I did this

As I've mentioned here and there, I've worked on a new Chevrotain-based Less parser for, I dunno, over five years off and on?

I'm finally getting to the part of comparing tests from the existing Less 1.x-4.x parser to what will (if everything goes well) become the Less 5.x+ parser.

However, I ran into a number of things that I wanted to refactor tests for.

  1. Many, many legacy tests had multiple concerns. That is, they tested multiple Less features at once. This has made checking parity difficult for features like CSS Nesting, where the Less 5.x parser would support CSS Nesting natively, and only "flatten" in certain cases. So, with days and days (weeks?) of ChatGPT / LLM prompting, I worked to simplify test files so that they covered fewer features per file.
  2. The test runner had implicit configurations bound to test files. So what I did was backport (somewhat) a Less 5.x+ feature where Less could be configured by a styles.config.{cjs, mjs, js, cts, mts, ts} file. That way, the Less 5 test runner could run Less files according to how their configuration settings.
  3. In the past, all test folders were duplicated in the less/ and css/ folders. This sometimes made updating tests tedious, as you'd have to find the copy for each. Instead, I co-located .css with their .less files, and grouped many of them into their own folders to make them easier to manage.

@less/maintainers @puckowski @iChenLei Let me know if this makes sense, and you have any questions or concerns!

@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Oct 27, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the Less.js test infrastructure to improve maintainability and prepare for a future Less 5.x parser. The main changes involve reorganizing test files, introducing configuration-based test execution, and modernizing the test runner infrastructure.

Key changes:

  • Introduces styles.config.cjs files for per-test configuration instead of hardcoded test options
  • Reorganizes test files into tests-unit/ and tests-config/ directories with co-located .css files
  • Updates test runner to use cosmiconfig for configuration loading and glob patterns for test discovery

Reviewed Changes

Copilot reviewed 59 out of 577 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/test-data/tests-error/eval/styles.config.cjs Adds config file for eval error tests with strict math/units and JS enabled
packages/test-data/tests-config/*/styles.config.cjs Multiple config files defining test-specific Less compiler options
packages/test-data/tests-config/url-args/urls.less Updates import paths to reflect new directory structure
packages/test-data/tests-config/static-urls/urls.* Updates paths and expected output for static URL tests
packages/less/test/less-test.js Major refactor to support glob patterns, cosmiconfig, and co-located test files
packages/less/test/index.js Replaces nock with needle mocking and updates test configuration to use glob patterns
packages/less/test/browser/* Updates browser test paths to reflect new directory structure
packages/less/scripts/postinstall.js Adds Playwright browser installation script for development environments
packages/less/package.json Updates dependencies and adds postinstall script
packages/less/Gruntfile.js Updates test paths to reflect new directory structure

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@puckowski
Copy link
Contributor

Seems there is an issue with Windows.

I changed:

cssPath = path.join(testFolder, 'css', css_name) + '.css';

to

cssPath = path.join(testFolder, '', css_name) + '.css';

in less-test.js as a quick and dirty workaround to run tests on my Windows machine.

master Less test coverage with c8:

=============================== Coverage summary ===============================
Statements   : 91.82% ( 11126/12117 )
Branches     : 92.59% ( 3327/3593 )
Functions    : 93.08% ( 673/723 )
Lines        : 91.82% ( 11126/12117 )
================================================================================

I cloned your branch and ran the same c8 command and got:

=============================== Coverage summary ===============================
Statements   : 88.57% ( 10733/12117 )
Branches     : 89.03% ( 3011/3382 )
Functions    : 91.97% ( 665/723 )
Lines        : 88.57% ( 10733/12117 )
================================================================================

Slight reduction in coverage. But pretty good given the size of the refactor!

This is the command I am using under packages/less for now:

npx c8 -r lcov -r json-summary -r text-summary `
  --include="lib/**/*.js" --include="dist/**/*.js" --include="bin/**/*.js" `
  npx grunt shell:test

I'll take a look at the diffs tomorrow, though I am curious as to what your stance on LLM-generated explainer Markdown files like CHANGES.md should be. My preference is to omit them unless they are critical to understand the code. If we let Markdown files slide in with each PR the repository could get messy.

@matthew-dean

@matthew-dean
Copy link
Member Author

@puckowski

I am curious as to what your stance on LLM-generated explainer Markdown files like CHANGES.md should be.

Oh, shoot, if they're in there, that was accidental. Sometimes the AI agents create them without asking.

Re: coverage -- thanks for putting that in there. Ideally, there wouldn't be a reduction. I'll see if I can add back in test cases for those missing lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants