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

data-test-id eslint rules, create function and cypress commands #54

Merged
merged 11 commits into from
Oct 11, 2019

Conversation

ojkelly
Copy link
Contributor

@ojkelly ojkelly commented Oct 10, 2019

This has 2 failing tests on the eslint rules, because i cannot get the eslint environment running correctly, for the tests to run.


Kablamo eslint plugin

yarn add -D @kablamo/eslint-plugin

Add the following to your .eslintrc.js.

module.exports = {
  plugins: ["@kablamo"],
  extends: ["plugin:@kablamo/recommended"],
};

Rules

test-id-prefix-match-path

This rule fixes the first argument of createTestIds(prefix, [ ...ids]) to be a prefix derived from the files
current directory. This ensures consistent unique prefixes per component.

You can still suffix variables to the end of your test id for example data-test-id={${testId.myId}-${uuid}}`.

test-id-consistent-naming

This rule ensures common misspellings always get fixed to the snake-case data-test-id.

Configurations

This plugin contains a reccomended elsint ruleset to enable the included rules.

@github-actions
Copy link

Coverage Status

Coverage increased (+0.05%) to 91.519% when pulling 3c7f566 on eslint-plugin into f916f47 on master.

@github-actions
Copy link

Coverage Status

Coverage increased (+0.05%) to 91.519% when pulling 3c7f566 on eslint-plugin into f916f47 on master.

@nhardy
Copy link
Member

nhardy commented Oct 10, 2019

This rule ensures common misspellings always get fixed to the snake-case data-test-id.

Should this be kebab-case data-testid?

packages/eslint-plugin/package.json Outdated Show resolved Hide resolved
packages/kerosene-test/package.json Outdated Show resolved Hide resolved
packages/kerosene-test/src/test-id/createTestIds.ts Outdated Show resolved Hide resolved
@ojkelly
Copy link
Contributor Author

ojkelly commented Oct 10, 2019

TODO: in another PR

adding the following commands to the kerosene-test cypress folder


Cypress.Commands.add("visitStorybook", (path, storybookName, options = {}) => {
  cy.visit(
    `iframe.html?selectedKind=${encodeURIComponent(
      path,
    )}&selectedStory=${encodeURIComponent(storybookName)}`,
    options,
  );
});

Cypress.Commands.add("testId", testId => {
  cy.get(`[data-test-id="${testId}"]`);
});

@ojkelly
Copy link
Contributor Author

ojkelly commented Oct 10, 2019

This rule ensures common misspellings always get fixed to the snake-case data-test-id.

Should this be kebab-case data-testid?

changed the doc to say kebab-case.

I've also seen data-test-id and data-testid bikeshedded.

I know react-testing-library was adamant about data-testid, but has since added the ability to specify a custom selector so you can use data-test-id.

The general guidelines around global data attributes say no caps https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/data-*. The spec itself says no uppers https://html.spec.whatwg.org/multipage/dom.html#embedding-custom-non-visible-data-with-the-data-*-attributes

And then finally, every example they have with more than one word has them separated by hyphens so data-one-two instead of data-onetwo.

For example, a library called "DoQuery" could use attribute names like data-doquery-range, and a library called "jJo" could use attributes names like data-jjo-range. The jJo library could also provide an API to set which prefix to use (e.g. J.setDataPrefix('j2'), making the attributes have names like data-j2-range).

That context along with my time in webdev makes data-test-id feel much more natural that data-testid, in a similar way that yml is preferable over yaml.

@ojkelly
Copy link
Contributor Author

ojkelly commented Oct 10, 2019

I've also added the cypress commands that make storybook and using data-test-id easier.

@ojkelly ojkelly changed the title Adding eslint rules and createTestIds data-test-id eslint rules, create function and cypress commands Oct 10, 2019
thoiberg
thoiberg previously approved these changes Oct 10, 2019
Copy link

@thoiberg thoiberg left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Nice work!

@nhardy
Copy link
Member

nhardy commented Oct 10, 2019

That context along with my time in webdev makes data-test-id feel much more natural that data-testid, in a similar way that yml is preferable over yaml.

Should it be configurable with data-test-id being the default then?

@ojkelly
Copy link
Contributor Author

ojkelly commented Oct 10, 2019

That context along with my time in webdev makes data-test-id feel much more natural that data-testid, in a similar way that yml is preferable over yaml.

Should it be configurable with data-test-id being the default then?

It could be. the eslint rule could take options ["data-test-id","data-testid"] the cypress helper command is harder. We could maybe make two files and you import the relevant one.

thoiberg
thoiberg previously approved these changes Oct 10, 2019
Co-Authored-By: Tim Hoiberg <tim.hoiberg@gmail.com>
packages/eslint-plugin/src/index.js Outdated Show resolved Hide resolved
thoiberg
thoiberg previously approved these changes Oct 10, 2019
plugins: ["@kablamo"],
extends: ["plugin:@kablamo/recommended"],
rules: {
"test-id-consistent-naming": ["data-testid"],
Copy link
Member

Choose a reason for hiding this comment

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

Should this be "test-id-consistent-naming": ["error", "data-testid"],?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i think so. the rulestester complains if you give it more than one arg, and I think that was confusing me.

@ojkelly ojkelly merged commit a714bfc into master Oct 11, 2019
@ojkelly ojkelly deleted the eslint-plugin branch October 11, 2019 01:10
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.

3 participants