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

Add new rule no-noop-setup-on-error-in-before #920

Merged
merged 1 commit into from
Aug 27, 2020

Conversation

v-korshun
Copy link

@v-korshun v-korshun commented Aug 19, 2020

Fixes #918

@bmish
Copy link
Member

bmish commented Aug 21, 2020

I will review this soon, thanks for submitting!

docs/rules/no-noop-setup-on-error-in-before.md Outdated Show resolved Hide resolved
docs/rules/no-noop-setup-on-error-in-before.md Outdated Show resolved Hide resolved
docs/rules/no-noop-setup-on-error-in-before.md Outdated Show resolved Hide resolved
docs/rules/no-noop-setup-on-error-in-before.md Outdated Show resolved Hide resolved
docs/rules/no-noop-setup-on-error-in-before.md Outdated Show resolved Hide resolved
lib/rules/no-noop-setup-on-error-in-before.js Outdated Show resolved Hide resolved
url:
'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-noop-setup-on-error-in-before.md',
},
fixable: 'code',
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit hesitant to make this rule autofixable by simply deleting the offending code. That changes the behavior of the code (which we normally try to avoid) and the author would likely need to make manual fixes anyway right? What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

There are certainly more actions that the author would have to do to fix the tests but it would vary on a case by case basis. The one thing that would have to be done anyway - removal of the offending code, then run the tests and see which test cases failed to add error handling in individual test. I would leave it for convenience.

Copy link
Member

Choose a reason for hiding this comment

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

@mongoose700 thoughts?

lib/rules/no-noop-setup-on-error-in-before.js Outdated Show resolved Hide resolved
lib/rules/no-noop-setup-on-error-in-before.js Outdated Show resolved Hide resolved
@bmish bmish changed the title Introduce no-noop-setup-on-error-in-before rule for tests Add new rule no-noop-setup-on-error-in-before Aug 25, 2020
@v-korshun v-korshun force-pushed the no-noop-setup-on-error-in-before branch 2 times, most recently from 895c702 to 7baf741 Compare August 26, 2020 00:39
@v-korshun v-korshun force-pushed the no-noop-setup-on-error-in-before branch from 7baf741 to 3da13a0 Compare August 26, 2020 04:57
Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

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

Thank you!

@bmish bmish merged commit 2a90f87 into ember-cli:master Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disallow noop setupOnerror in before/beforeEach
2 participants