Skip to content

Conversation

@RobinVdBroeck
Copy link
Contributor

@RobinVdBroeck RobinVdBroeck commented Oct 8, 2025

PR Checklist

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
Copy link

linux-foundation-easycla bot commented Oct 8, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: RobinVdBroeck / name: Robin Van den Broeck (79e2adf, a856d6e)

@mark-wiemer mark-wiemer added the semver-major implementation requires increase of "major" version number; "breaking changes" label Oct 11, 2025
@mark-wiemer
Copy link
Member

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
Copy link
Member

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 RobinVdBroeck changed the title Change the default of --forbid-only to check for process.env.CI feat!: change the default of --forbid-only to check for process.env.CI Oct 16, 2025
@RobinVdBroeck
Copy link
Contributor Author

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.

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

Labels

semver-major implementation requires increase of "major" version number; "breaking changes"

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🚀 Feature: Exit with code 1 when .only is used by default in CI (--forbid-only)

3 participants