feat!: change the default of --forbid-only to check for process.env.CI

[RobinVdBroeck]

PR Checklist

• Addresses an existing open issue: fixes 🚀 Feature: Exit with code 1 when .only is used by default in CI (--forbid-only) #5299
• That issue was marked as status: accepting prs
• Steps in CONTRIBUTING.md were taken

Overview

I've changed the default value of --forbid-only to be the truthy value of process.env.CI.

Since this is a major change, I've added documentation for the behaviour before v12 (the next major release) and after. Let me know if this is not how docs are written for mocha, and how I should change it to be inline with mocha's standards.

I could use some pointers on where to add tests. I've only found integration level tests for testing CLI flags, but that seems kinda overkill for adding a default? Should I write an unit test somewhere, and if so, where?

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: mark-wiemer / name: Mark Wiemer (5e8d11a)

[mark-wiemer]

Thanks @RobinVdBroeck for this one :) unfortunately from what I'm seeing, our exports.builder function is a pretty tight wrapper around yargs, which I'm familiar with but not an expert in. Right now, I don't know how to test custom values being passed into this builder, it seems to always read from process.argv, so we can't test it in a unit test without significant refactoring, and I agree that an integration test feels like overkill.

Curious if @mochajs/maintenance-crew has any insights here, otherwise this may sit for a bit while I work on other bugfixes and think about this one

[JoshuaKGoldberg]

Yeah, I think it's fine to omit testing for this one. The behavioral change is one line. Tests would be ideal but 🤷 it's old code locked into a design pattern we don't control.

[RobinVdBroeck]

I've pushed 2 new commits:

• One fixes the existing integration tests that depend on .only, since these now fail while being run in CI. I missed this in my original PR since I did not run tests with CI=true 😅
• I've added that --forbid-only is the default in CI environments to the error message. Let me know if this is too much, I was on the fence myself, and I'll drop the commit again.

[mark-wiemer]

Hi @RobinVdBroeck would you mind resolving the new conflicts? They should just be format fixes, npm run format:prettier should resolve most of them :)

The new message looks good to me, thank you

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.70%. Comparing base (fb0215b) to head (5e8d11a).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5496      +/-   ##
==========================================
+ Coverage   93.69%   93.70%   +0.01%     
==========================================
  Files          57       57              
  Lines        4390     4397       +7     
  Branches      849      849              
==========================================
+ Hits         4113     4120       +7     
  Misses        277      277              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mark-wiemer]

Thank you!