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

feat: add MOCHA_OPTIONS env variable #4835

Merged
merged 3 commits into from
Jun 4, 2024
Merged

Conversation

icholy
Copy link
Contributor

@icholy icholy commented Feb 25, 2022

Description of the Change

The MOCHA_OPTIONS environment variable is a space-separated list of command-line options. It essentially copies node's approach with NODE_OPTIONS

The config priority is:

  1. Command-line args
  2. MOCHA_OPTIONS environment variable.
  3. RC file (.mocharc.c?js, .mocharc.ya?ml, mocharc.json)
  4. mocha prop of package.json
  5. default configuration (lib/mocharc.json)

Alternate Designs

Yargs has support for env variables: https://yargs.js.org/docs/#api-reference-envprefix
However, using this feature would result in a separate env variable for each option:

Ex:

  • MOCHA_BAIL
  • MOCHA_ALLOW_UNCAUGHT
  • MOCHA_FAIL_ZERO
  • etc ...

I decided to to go with MOCHA_OPTIONS because it seems less intrusive and that's how node does it too.

Why should this be in core?

I don't see how it could live anywhere else.

Benefits

My specific use-case is specifying MOCHA_OPTIONS='--inspect-brk' when calling npm run test from vim.

Possible Drawbacks

  • More code is always a liability.
  • More config sources can make things more difficult to debug.

Applicable issues

Fixes #4330.

@icholy
Copy link
Contributor Author

icholy commented Mar 18, 2022

@juergba have you had a chance to look this over?

@github-actions
Copy link
Contributor

This PR hasn't had any recent activity, and I'm labeling it stale. Remove the label or comment or this PR will be closed in 14 days. Thanks for contributing to Mocha!

@github-actions github-actions bot added the stale this has been inactive for a while... label Jul 18, 2022
@outsideris outsideris removed the stale this has been inactive for a while... label Jul 31, 2022
@github-actions
Copy link
Contributor

This PR hasn't had any recent activity, and I'm labeling it stale. Remove the label or comment or this PR will be closed in 14 days. Thanks for contributing to Mocha!

@github-actions github-actions bot added the stale this has been inactive for a while... label Nov 30, 2022
@github-actions github-actions bot closed this Dec 16, 2022
@icholy
Copy link
Contributor Author

icholy commented Sep 20, 2023

@outsideris I still need this feature. Would I be able to get some feedback?

@JoshuaKGoldberg
Copy link
Member

👋 coming back to this @icholy, are you still interested in working on this PR? As of #5027 there are maintainers who can review it now. No worries if you no longer have time - but if you do that'd be great!

@icholy
Copy link
Contributor Author

icholy commented Dec 2, 2023

Yes!

Copy link

linux-foundation-easycla bot commented Dec 2, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: icholy / name: Ilia Choly (d986a9a, a2868fe)
  • ✅ login: voxpelli / name: Pelle Wessman (4b87ce6)

@JoshuaKGoldberg
Copy link
Member

Yay! We're not quite ready to start reviewing PRs yet, but hope to soon.

Note that I don't see any linked issues. The contributing guidelines right now ask for that so that folks can discuss the change first. We'll continue to require that after #5038. Was there any impetus for this PR besides what's in the description?

@coveralls
Copy link

coveralls commented Dec 2, 2023

Coverage Status

coverage: 94.359% (+0.001%) from 94.358%
when pulling 4b87ce6 on icholy:master
into 472a8be on mochajs:master.

@icholy
Copy link
Contributor Author

icholy commented Dec 2, 2023

There's no separate issue, the PR description contains all the context I have. I can create an issue if you'd like though.

@JoshuaKGoldberg
Copy link
Member

That would be lovely, yes please!

Note that the three of us new maintainers are still ramping up on the project, so it might be some weeks (or even months 😞) before we have enough context to really triage the issue and review this PR. We'll do our best to get to it soon though!

@icholy
Copy link
Contributor Author

icholy commented Dec 3, 2023

That's alright. It's been two years, what's another month or two?

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Sweet! Really pleasing to see such a clean implementation. And with thorough tests! 👏

Just requesting changes on docs.

lib/cli/options.js Show resolved Hide resolved
lib/cli/options.js Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added status: waiting for author waiting on response from OP - more information needed semver-minor implementation requires increase of "minor" version number; "features" and removed stale this has been inactive for a while... labels Mar 4, 2024
@icholy icholy force-pushed the master branch 2 times, most recently from ce9ba6b to d79e034 Compare March 7, 2024 04:04
@JoshuaKGoldberg JoshuaKGoldberg removed the status: waiting for author waiting on response from OP - more information needed label Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor implementation requires increase of "minor" version number; "features" status: in triage a maintainer should (re-)triage (review) this issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 Feature: Support setting options via environment vars
6 participants