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

Forbid only in mocha #2493

Merged
merged 4 commits into from
Oct 22, 2019
Merged

Forbid only in mocha #2493

merged 4 commits into from
Oct 22, 2019

Conversation

leomoty
Copy link
Contributor

@leomoty leomoty commented Oct 22, 2019

I saw #2489 and was thinking, how about forbidding .only in Mocha for CI only? Should be way easier to catch if someone mistakenly commits it.

 mocha "**/*.api.js" --headless --forbid-only
C:\Users\leomo\Documents\projects\xterm.js\node_modules\yargs\yargs.js:1163
      else throw err
           ^

Error: `.only` forbidden

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

😮 this is awesome! Had no idea this was a thing.

We should also add it for the yarn test-unit above, it probably needs to be passed through inside bin/test.js for that.

@Tyriar Tyriar added this to the 4.2.0 milestone Oct 22, 2019
@leomoty
Copy link
Contributor Author

leomoty commented Oct 22, 2019

How do you wanna handle that? It seems that argv.length > 2 is considered as test files, is this feature used anywhere?

@Tyriar
Copy link
Member

Tyriar commented Oct 22, 2019

Yeah @jerch added that, how about using the same flag name and doing the test file check by filtering out flag args: nonFlagArgs = argv.filter(e => e.startsWith('--'))?

@leomoty leomoty requested a review from Tyriar October 22, 2019 03:01
@leomoty
Copy link
Contributor Author

leomoty commented Oct 22, 2019

I hope I understood you correctly, I think you meant flagArgs there. This should work, by the way.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

👌

Thanks for looking into this, will prevent accidentally merging .only again 😅

@Tyriar Tyriar merged commit 15392c6 into xtermjs:master Oct 22, 2019
@leomoty leomoty deleted the forbid-only branch October 22, 2019 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants