Skip to content

Cypress eslint fixes and config#3636

Merged
ranbena merged 10 commits into
masterfrom
cypress-eslint
Mar 25, 2019
Merged

Cypress eslint fixes and config#3636
ranbena merged 10 commits into
masterfrom
cypress-eslint

Conversation

@ranbena

@ranbena ranbena commented Mar 24, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • Feature

Description

  1. Added cypress folder to lint script.
    • updated npm run lint to run on ./client instead of ./client/app.
    • added **/dist to .eslintignore
    • apparently eslint never detected the ignore file cause it's not in project root 😵 so added --ignore-path ./client/.eslintignore. Now there's also no need for lint:staged specific folder ignoring.
    • removed node_modules from ignore file cause it's ignored by default.
  2. Cypress specific eslint config
    • Created eslintrc in cypress folder (eslint cascading feature).
    • Removed cypress stuff from main eslintrc.
    • Added cypress lint plugin that enforces cypress best practices.
    • Allowing chai assertions like expect(element).to.be(true).
    • Moved jest plugin to ./client/app cause it collided with chai syntax.
  3. Fixed lint errors and warnings
    • fixed some code
    • disabled lint where necessary
    • disabled func-name lint to allow anonymous functions it('test', function () {.

Tested on npm run lint, lint-staged and vscode.

@ranbena
ranbena requested review from arikfr and gabrieldutra March 24, 2019 19:17
@ranbena
ranbena force-pushed the cypress-eslint branch 2 times, most recently from f0f3e29 to 5ef1cbe Compare March 24, 2019 19:56
@ranbena

ranbena commented Mar 24, 2019

Copy link
Copy Markdown
Contributor Author

Looks like codeclimate complies with the root eslintrc but ignores the rest 😩
Could it be that this line is revoking the cascading feature?:

redash/.codeclimate.yml

Line 27 in 25910e7

config: client/.eslintrc.js

Might have been said here.

@arikfr @gabrieldutra wdyt?

@ranbena

ranbena commented Mar 25, 2019

Copy link
Copy Markdown
Contributor Author

Perhaps we can do linting from a CircleCI step instead of CodeClimate?

@arikfr

arikfr commented Mar 25, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

The issue you linked to mentions:

We do support cascading .eslintrc* files in effectively the same way that vanilla ESLint supports them. Does that clear it up a bit?

Maybe we need to remove the explicit eslint config setting and let it find on its own?

@ranbena

ranbena commented Mar 25, 2019

Copy link
Copy Markdown
Contributor Author

lol it worked

@ranbena

ranbena commented Mar 25, 2019

Copy link
Copy Markdown
Contributor Author

I'm gonna test it by pushing a lint error.

@ranbena

ranbena commented Mar 25, 2019

Copy link
Copy Markdown
Contributor Author

No good. There should be 2 errors showing up, but CodeClimate is oblivious @arikfr

@arikfr

arikfr commented Mar 25, 2019

Copy link
Copy Markdown
Member

It seems to ignore some plugins.

@ranbena

ranbena commented Mar 25, 2019

Copy link
Copy Markdown
Contributor Author

I see 3 ways to go about this:

  1. Understand and fix CodeClimate config.
  2. Switch to CircleCi step for linting (not tested).
  3. Merge all rules into the main eslintrc.

Gonna try #3 now.

@ranbena

ranbena commented Mar 25, 2019

Copy link
Copy Markdown
Contributor Author

@arikfr so I narrowed it down to a working config:

  1. Back to one ./client/eslintrc.js.
  2. Can't use plugin:cypress/recommended - causes CodeClimate error. Helpful lints but not a must.

Good to go?

gabrieldutra
gabrieldutra reviewed Mar 25, 2019
View reviewed changes
Comment thread client/cypress/integration/dashboard/dashboard_spec.js Outdated
schedule: null,
}, data);

// eslint-disable-next-line cypress/no-assigning-return-values

@gabrieldutra gabrieldutra Mar 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

cypress/recommended seemed nice 😕. As we can't use it, should those cypress eslint-disable be removed?

@ranbena ranbena Mar 25, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's cause the cypress package isn't a required installation.

@arikfr arikfr Mar 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be 🤔

@arikfr

arikfr commented Mar 25, 2019

Copy link
Copy Markdown
Member

Good to go?

While I liked the fact you split the configuration, the current version is better than what we had until now so good to go. 👍

@ranbena
ranbena merged commit ec4b36b into master Mar 25, 2019
@ranbena
ranbena deleted the cypress-eslint branch March 25, 2019 20:14
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 2019
@ranbena @harveyrendell
Cypress eslint fixes and config (getredash#3636)
300a653
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gabrieldutra gabrieldutra gabrieldutra left review comments
@arikfr arikfr Awaiting requested review from arikfr

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ranbena @arikfr @gabrieldutra