test: separate coverage runs from default tests and replace Makefile

[lumirlumir]

Prerequisites checklist

• I have read the contributing guidelines.

AI acknowledgment

• I did not use AI to generate this PR.
• (If the above is not checked) I have reviewed the AI-generated content before submitting.

What is the purpose of this pull request?

This PR separates coverage runs from the default tests, as mentioned in #747 (comment).

It mostly follows the same strategy used in eslint/rewrite#357.

What changes did you make? (Give an overview)

The current version of the PR mainly focuses on avoiding a large git diff. However, I have two questions:

• Should the Makefile.js for eslint-scope include the mocha test runner script? I think mocha could be defined directly in the scripts section of package.json instead of being consolidated into Makefile.js, since this is the only package that uses Makefile.js, unlike eslint-visitor-keys and espree.

• Should we use --color --reporter progress in packages/espree/package.json? The language plugins and rewrite projects also don’t use this flag. I think we could consolidate test:cjs and test:esm into a test:unit script without this flag.

Related Issues

Ref: #747 (comment)

Is there anything you'd like reviewers to focus on?

N/A

[DMartens]

To your questions:

• I agree that removing the Makefile makes sense. As you said the test could be defined inline and the checkLicenses script could be moved into tools/check-licenses.js
• Both of the links do not show mocha using the mentioned CLI flags (maybe you modified them locally?). To the suggestion itself: I think having better defaults is worthwile but this should be uniform for all repositories

[DMartens]

Suggested change

	"test:unit": "mocha \"tests/**/*.test.cjs\" && mocha \"tests/**/*.test.js\""
	"test:unit": "mocha \"tests/**/*.test.cjs\" \"tests/**/*.test.js\""

Is there a reason to split the run of tests?

[lumirlumir]

It was originally kept to reduce unnecessary diffs, but your suggestion makes sense, I’ve updated it as well in 6185414.

[lumirlumir]

I agree that removing the Makefile makes sense. As you said the test could be defined inline and the checkLicenses script could be moved into tools/check-licenses.js

I’ve updated it accordingly in 6185414. I also removed the unnecessary shelljs dev dependency, since it was only used in a small part and can be replaced with native tools.

Both of the links do not show mocha using the mentioned CLI flags (maybe you modified them locally?). To the suggestion itself: I think having better defaults is worthwile but this should be uniform for all repositories

Oops, I think my English was unclear. To clarify, I meant we should simply remove the --color --reporter progress --timeout 30000 flags, since they aren’t generally used across the repositories.

I removed these flags in 6185414, but please let me know if you have any other ideas.

For --timeout 30000, in markdown, it was kept due to an issue involving a super-linear regular expression and potential ReDoS concerns. However, in this repo, that timeout doesn’t seem necessary, so I removed it.

js/packages/espree/package.json

Line 66 in db0b9e4

"test:cjs": "mocha --color --reporter progress --timeout 30000 \"tests/**/*.test.cjs\"",

[DMartens]

The removal of shelljs and extracting the license check script from the Makefile.js are big wins. As the PR no longer only updates the test scripts, we should update the PR title, e.g. "refactor: use package.json scripts over Makefile"

I do not have a strong opinion on setting defaults for the test runner such as --color or --reporter as I think the developer should choose for themselves how the test output should look. As such I agree with removing them, making it consistent with the other repositories.

Regarding the timeout: if the problem pops up again, we should rather use test suite / case scoped timeouts rather than a global one (the global timeout may hide other problems)

[lumirlumir]

Friendly ping @DMartens

[DMartens]

Changes LGTM, thanks. Sorry for the delay.