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

#1818: implement --production flag #1935

Closed

Conversation

aaron-j-krause
Copy link
Contributor

Addresses #1818 and #1759.

Adds a --production flag that fails tests that are: pending, marked as skipped, marked as only, or have a duplicate title.

Since only is determined by grep the only way I could think to catch them is to fail a test if it sees a grep. Since this is, in a way, a linter meant to be run over all the tests before a commit or the like I figured that wouldn't be an issue. At the moment it only catches at the test level, so it won't catch suites marked as skipped or only.

Review on Reviewable

Stricter testing mode. Fails tests that are: pending, marked as skipped, marked as only, have duplicate title
@aaron-j-krause
Copy link
Contributor Author

Any thoughts on this?

@danielstjules
Copy link
Contributor

Thanks for the PR! :)

@boneskull what do you think?

@glenjamin
Copy link
Contributor

My 2¢ - I'd want to fail on only or duplicate titles, but not on pending or skipped.

Only is something that should never be committed, if it's present then it must be a mistake (like leaving in a debugger statement).

Pending or skipped I'll sometimes use to allow merging code which will later need updates - I can configure CI tools to count these and show builds as unstable if needed.

@dasilvacontin
Copy link
Contributor

dasilvacontin commented Apr 13, 2016

My 2¢ - I'd want to fail on only or duplicate titles, but not on pending or skipped.

Same here.

Only is something that should never be committed, if it's present then it must be a mistake (like leaving in a debugger statement).

If I commit a .only or a debugger statement is for debugging purposes, or because it's a WIP commit – I want CI to fail the build.

I'm not fond of the flag's name, production, since that's not the condition under which I want .onlys to fail the build. I also want them to fail the build in PRs, in the dev branch, in the feature branch, in staging, etc. Probably everywhere but locally. So it would always make CI fail.

I wonder if there's anyone who doesn't want CI to fail when tests have .only, and why aren't they using pending tests instead. Maybe to make a CI build shorter when they are only interested on the output of certain test? dunno

@glenjamin
Copy link
Contributor

I'm not fond of the flag's name, production, since that's not the condition under which I want .onlys to fail the build. I also want them to fail the build in PRs, in the dev branch, in the feature branch, etc. Probably everywhere but locally. So it would always make CI fail.

Perhaps --strict ?

@aaron-j-krause
Copy link
Contributor Author

IIRC 'strict' was the original idea for the flag but 'production' was suggested in order to avoid similarities to 'strict mode' which it isn't really related to. Taking pending and skip filtering out of this PR would be easy enough. I could see the case for leaving pending in. Not so sure about skips, though.

@dasilvacontin
Copy link
Contributor

IMO skip is for temporally (days, weeks, hopefully not more) disabling some tests, because they are no longer correct, they have to be reworked, the code they are covering is undergoing changes and I don't care about it being covered or not, whatever. So I want those to be committed, and not to break the build.

Sadly, I still don't have a better naming proposal, but I'll continue thinking about it.

On 15 Apr 2016, at 23:41, aaroncrows notifications@github.com wrote:

IIRC 'strict' was the original idea for the flag but 'production' was suggested in order to avoid similarities to 'strict mode' which it isn't really related to. Taking pending and skip filtering out of this PR would be easy enough. I could see the case for leaving pending in. Not so sure about skips, though.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub

@boneskull
Copy link
Member

👎 @aaroncrows is this suitable for the use case?

@boneskull
Copy link
Member

A feature like this may be helpful, but since there's a (arguably better) workaround via ESLint, I'm not in a hurry to merge this...

@boneskull
Copy link
Member

it doesn't cover skip (I think), but could be trivially extended to do so

@hollomancer
Copy link

This can be closed, due to changes in 3.0.0 in how the internals of it work. The original issue of #1818 needs a new PR.

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.

None yet

6 participants