-
Notifications
You must be signed in to change notification settings - Fork 0
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
chore(stylelint-plugin): setup vitest [KHCP-10186] #213
base: main
Are you sure you want to change the base?
Conversation
/** | ||
* PostCSS AST declaration node | ||
* Docs: https://postcss.org/api/#declaration | ||
*/ | ||
|
||
const declProp = decl.prop | ||
const declValue = decl.value | ||
// check if the value contains a token as CSS or SCSS variable | ||
const hasToken = [`--${KONG_TOKEN_PREFIX}`, `$${KONG_TOKEN_PREFIX}`].some(pattern => declValue.includes(pattern)) | ||
if (!hasToken) { | ||
// skip validating if the value does not contain a token | ||
return | ||
} | ||
|
||
const tokenProperty = Object.keys(PROPERTY_TOKEN_MAP).find(key => key.split(',').some(prop => prop === declProp)) | ||
|
||
// check if the property is in the property map | ||
const isEnforcedProp = !!tokenProperty | ||
if (!isEnforcedProp) { | ||
return | ||
} | ||
|
||
const valueTokens = extractTokensFromValue(declValue) | ||
// get the appropriate tokens for the property and create regex for each | ||
// regex pattern: /kui-(?:[a-z0-9-]+-)?token(?:-[a-z0-9-]+)?$/ | ||
// this allows to match both regular and component tokens | ||
const appropriateTokens = PROPERTY_TOKEN_MAP[tokenProperty].map(token => new RegExp(KONG_TOKEN_PREFIX + '(?:[a-z0-9-]+-)?' + token + '(?:-[a-z0-9-]+)?$')) | ||
// filter out tokens that are not appropriate for the property | ||
const inappropriateTokens = valueTokens.filter(vToken => !appropriateTokens.some(aTokenRegex => aTokenRegex.test(vToken))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part (the rule logic) was moved to a separate file
stylelint-plugin/package.json
Outdated
@@ -0,0 +1,15 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: this isn't a standalone package, so it shouldn't have its own dependencies or commands.
Move the commands to the root package.json, and install the devDependencies there, if needed
.github/workflows/test.yaml
Outdated
yarn --cwd stylelint-plugin install --frozen-lockfile | ||
yarn --cwd stylelint-plugin test:unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependencies are resolved from the root
yarn --cwd stylelint-plugin install --frozen-lockfile | |
yarn --cwd stylelint-plugin test:unit | |
yarn test:unit |
@@ -0,0 +1,103 @@ | |||
import { describe, it, expect } from 'vitest' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: rename test file to match the file it tests with suffix *.spec.ts
@@ -0,0 +1,38 @@ | |||
const PROPERTY_TOKEN_MAP = require('./token-map') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue/nit: all other files are kebab-case; can you rename this one to match?
expect(getInappropriateTokens(decl)).toEqual([]) | ||
}) | ||
|
||
it('returns empty array when no `kui-` token used in value (CSS variable)', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're going to have two separate sets of tests CSS Custom Properites
and SCSS variables
can you add two child describe
blocks within the root to separate the tests?
describe('CSS Custom Properties, () => {})
describe('SCSS variables, () => {})
it('returns empty array when property is not enforced', () => { | ||
const decl = { | ||
prop: 'border-collapse', | ||
value: 'var(--kui-color-text)', | ||
} | ||
|
||
expect(getInappropriateTokens(decl)).toEqual([]) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand what "is not enforced" means here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rephrased (property that is not guarded)
it('returns empty array when appropriate component token used', () => { | ||
const decl = { | ||
prop: 'color', | ||
value: 'var(--kui-method-get-color-text)', | ||
} | ||
|
||
expect(getInappropriateTokens(decl)).toEqual([]) | ||
}) | ||
|
||
it('returns an array with token when inappropriate component token used (CSS variable)', () => { | ||
const decl = { | ||
prop: 'color', | ||
value: 'var(--kui-method-get-color-background)', | ||
} | ||
|
||
expect(getInappropriateTokens(decl)).toEqual(['kui-method-get-color-background']) | ||
}) | ||
|
||
it('returns an array of unique tokens when inappropriate component token used (CSS variable with SCSS fallback)', () => { | ||
const decl = { | ||
prop: 'color', | ||
value: 'var(--kui-method-get-color-background, $kui-method-get-color-background)', | ||
} | ||
|
||
expect(getInappropriateTokens(decl)).toEqual(['kui-method-get-color-background']) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably move these into a component tokens
describe block
Install the preview package from this PR in your consuming applicationIn your host project, you may install the preview package version generated by this PR: @kong/design-tokens@pr-213 |
@@ -45,6 +45,9 @@ jobs: | |||
- name: Lint | |||
run: yarn lint | |||
|
|||
- name: Run unit tests | |||
run: yarn test:unit:coverage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"test:unit": "vitest", | ||
"test:unit:open": "vitest --ui", | ||
"test:unit:coverage": "vitest run --coverage" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: since we won't have another type test here, let's update the commands to just test
"test:unit": "vitest", | |
"test:unit:open": "vitest --ui", | |
"test:unit:coverage": "vitest run --coverage" | |
"test": "vitest", | |
"test:open": "vitest --ui", | |
"test:coverage": "vitest run --coverage" |
@@ -45,6 +45,9 @@ jobs: | |||
- name: Lint | |||
run: yarn lint | |||
|
|||
- name: Run unit tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
- name: Run unit tests | |
- name: Run tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: should the test step be after the build?
"@vitest/coverage-v8": "^1.4.0", | ||
"@vitest/ui": "^1.4.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: you added dependencies but I don't see any lockfile changes
@@ -0,0 +1,38 @@ | |||
const PROPERTY_TOKEN_MAP = require('./token-map') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: in the plugin files that changed, it's hard to compare to the original files. Other than moving them, are there any other changes?
/** | ||
* @param {Object} decl PostCSS AST declaration node (https://postcss.org/api/#declaration) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add a description of the function
/** | |
* @param {Object} decl PostCSS AST declaration node (https://postcss.org/api/#declaration) | |
/** | |
* DESCRIPTION_GOES_HERE | |
* @param {Object} decl PostCSS AST declaration node (https://postcss.org/api/#declaration) |
return inappropriateTokens | ||
} | ||
|
||
export default getInappropriateTokens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: this is esm syntax; shoudn't this be CommonJS with module.exports
?
it('returns empty array when token is used for a property that is not guarded', () => { | ||
const decl = { | ||
prop: 'border-collapse', | ||
value: 'var(--kui-color-text)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: For some of your tests, sometimes you pass in a raw value, other times you pass in a CSS Custom Property (like shown here) and other times a Sass variable.
The type of input you pass in doesn't seem to matter in most of the tests, based on their descriptions, so is there a reason we can't standardize and just pass in raw values, e.g. red
or #000
?
When the value changes formats randomly between tests, it makes it seem like the value is part of what is being tested, even though it is not.
If you do want to run the unit tests for each type of value (e.g. raw, CSS custom property, Sass variable) we can define an array at the top of the test file and loop through each value to test each input type.
Summary
Adds unit tests for stylelint plugin rules logic
Jira: https://konghq.atlassian.net/browse/KHCP-10186