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.

Sign up for GitHub

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

Jump to bottom

feat: make forbid-only as default on CI #3987

Closed
wants to merge 3 commits into from
Closed

feat: make forbid-only as default on CI #3987

wants to merge 3 commits into from

Conversation

JaeHyeonKim19
Copy link

Description of the Change

  • When process.env.CI is true, forbid-only : true is default value. And I changed one of conditional statements in package-scripts.js to keep its function.

Alternate Designs

  • There're may be a better way, but it's my best. 😂

Why should this be in core?

  • Due to this change, We can prevent pushing to remote repo with .only.

Benefits

  • Due to this change, We can prevent pushing to remote repo with .only.

Possible Drawbacks

  • If this change apply to mocha, Existing users can be confused. Because project which use '.only' on purpose gonna make conflict on CI. So We've to announce about this change when this change is applied.

Applicable issues

@jsf-clabot
Copy link

jsf-clabot commented Aug 16, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@outsideris outsideris added semver-major implementation requires increase of "major" version number; "breaking changes" area: usability concerning user experience or interface labels Aug 16, 2019
@outsideris
Copy link
Contributor

Tests which use a fixture that has .only are failed. In Ci, we should handle this.

@JaeHyeonKim19
Fix failed test cases(mochajs#3030)
fcdb971 
Test cases broken because of making forbid-only as default in CI..
@coveralls
Copy link

coveralls commented Oct 26, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 93.594% when pulling 80e75c8 on JaeHyeonKim19:issue/3030 into 63eb80b on mochajs:master.

@JaeHyeonKim19
Copy link
Author

Any review😂?

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Dec 2, 2023 •
edited
Loading

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

(I do see that your last message was asking for review - unless directed otherwise we'll try to review it once we're ramped up enough!)

JoshuaKGoldberg
JoshuaKGoldberg requested changes Mar 1, 2024
View reviewed changes
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.

The functional default value change looks good to me! Requesting changes I'm not sold on the --no-forbid-only, and would like to see docs. Thanks! 🙌

(if you don't have time for this, we'll go ahead and send a new PR after a while)

package-scripts.js
if (!/^only-/.test(testName)) {
mochaParams += ' --forbid-only';
if (/^only-/.test(testName)) {
mochaParams += ' --no-forbid-only';
Copy link
Member

Choose a reason for hiding this comment

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

[Question] 🤔 Is --no-forbid-only necessary? My interpretation of #3030 (comment) was that the issue can be resolved with just the change to forbid-only's default value.

lib/cli/run.js
Copy link
Member

Choose a reason for hiding this comment

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

[Docs] https://mochajs.org/#-forbid-only should be updated to mention this, too. Folks will want to know if the default value isn't always falsy.

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Mar 1, 2024
@JoshuaKGoldberg JoshuaKGoldberg changed the title make forbid-only as default on CI(#3030) feat: make forbid-only as default on CI Mar 4, 2024
@JoshuaKGoldberg JoshuaKGoldberg added the stale this has been inactive for a while... label Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoshuaKGoldberg JoshuaKGoldberg JoshuaKGoldberg requested changes

Assignees
No one assigned
Labels
area: usability concerning user experience or interface semver-major implementation requires increase of "major" version number; "breaking changes" stale this has been inactive for a while... status: waiting for author waiting on response from OP - more information needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make --forbid-only (but not --forbid-pending) default
5 participants
@JaeHyeonKim19 @jsf-clabot @outsideris @coveralls @JoshuaKGoldberg