-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
test: Fix test descriptions and enforce it via new linting rule #11285
test: Fix test descriptions and enforce it via new linting rule #11285
Conversation
243c6e5
to
4216d78
Compare
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.
Marking this as draft because it will conflict with v26 merge later today
.eslintrc.js
Outdated
@@ -3,6 +3,7 @@ module.exports = { | |||
env: { | |||
node: true, | |||
}, | |||
// plugins: ['@renovate'], |
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.
can we references this via giturl as dependency until we release it to npm / github pacakges?
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 kind of learning towards leaving it as git-only, because we will be the only user
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.
So we should add a v0.0.1
tag to renovatebot/eslint-plugin
and reference it in package.json
as
"@renovate/eslint-plugin": "renovatebot/eslint-plugin#v0.0.1"
We can also just use the commit hash for now.
"@renovate/eslint-plugin": "renovatebot/eslint-plugin#1ebc0b1"
lib/versioning/cargo/index.spec.ts
Outdated
@@ -1,16 +1,15 @@ | |||
import { api as semver } from '.'; | |||
|
|||
describe('semver.matches()', () => { |
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.
better add a new nesting level, so we can have function name as second level describe block
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.
In this PR, I've made minimal merge into single top-level describe
-block. Later, I would like to gradually refactor all versioning tests to more readable table form, as I did for Cargo. And I'm about to start from version schemes you mentioned 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.
I would prefer to add a new toplevel describe block here instead, i can ignore whitespace changes at review, so we have only a few changed lines.
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.
Ah, now I got what you wanted. Just was confused with it()
that doesn't allow duplicates and thought nested describe
would raise similar lint warnings too.
…fix-test-descriptions
…fix-test-descriptions
BTW, usage of this linter implies that you can have only one root |
Fixed Windows CI for |
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.
We should completly remove export function getName(): string
function or is it used anywhere else?
Otherwise LGTM
@@ -1469,6 +1469,10 @@ | |||
dependencies: | |||
"@octokit/openapi-types" "^9.5.0" | |||
|
|||
"@renovate/eslint-plugin@https://github.com/renovatebot/eslint-plugin#v0.0.3": | |||
version "0.0.1" |
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.
You should update the version next time 😉
🎉 This PR is included in version 26.1.3 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Changes:
*.spec.ts
files.Context:
Closes #11198
Documentation (please check one with an [x])
How I've tested my work (please tick one)
I have verified these changes via: