Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: add new packagerule-tester#6777

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
bradzacher merged 13 commits intov6fromfork-rule-tester
Apr 27, 2023
Merged

feat: add new packagerule-tester#6777

bradzacher merged 13 commits intov6fromfork-rule-tester
Apr 27, 2023

Conversation

bradzacher
Copy link
Member

@bradzacherbradzacher commentedMar 27, 2023
edited
Loading

PR Checklist

References:

Overview

The changes I implemented in#5750 were pretty complex to build because I was working around how the baseRuleTester class did things. I ran into a lot of weirdness due to things like the class expecting specific properties on objects.

In looking to implement#6499 - I don't want to hit the same issues, so I think it's best if we just draw a line in the sand and fork it so that we can build our enhancements directly into the tooling.

This is also good because it means that we can provide a new baseline for testing and don't need to worry about what the installed ESLint version supports - which will make our eventual dependency testing easier.

This PR:

  • fork the base class and adds types
    • The logic itself is unchanged, but I did move some things around to make it a bit easier to type and manage.
  • clones the base eslint test forRuleTester
    • This test is completely unchanged because it'll take a complete rewrite to make it jest-compatible.
  • adds the changes we've added in our existing subclass fork:
    • addsafterAll as additional static "test framework providers" to the class.
    • adds a new per-test option -skip which runs the test usingit.skip.
    • adds automatic parser cache cleaning usingafterAll.
    • adds automatic file naming for type-aware linting
      • I implemented this slightly differently than before:
        • it uses a different default name fortsx files now
        • the default filenames can be overridden in the constructor (previously you HAD to do it per test).
    • adds per-test dependency constraints
      • I implemented this slightly differently than before:
        Before our hands were tied and I had to lean on settingonly on every test to filter out the skipped tests - now with the newskip functionality we only need setskip on these tests which is a much, much nicer implementation!
  • adds new features
    • adds a new per-test option -skip which runs the test case usingit.skip.
    • addsitSkip anddescribeSkip as additional static "test framework providers" to the class.
    • adds a newdefaultFilenames constructor option to allow configuring the default filenames used for type-aware testing.

Documentation:
https://deploy-preview-6777--typescript-eslint.netlify.app/architecture/rule-tester

TODO:

armano2 and JoshuaKGoldberg reacted with heart emoji
@bradzacherbradzacher added enhancementNew feature or request DO NOT MERGEPRs which should not be merged yet labelsMar 27, 2023
@typescript-eslint
Copy link
Contributor

Thanks for the PR,@bradzacher!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently onhttps://opencollective.com/typescript-eslint.

@netlify
Copy link

netlifybot commentedMar 27, 2023
edited
Loading

Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commitd5a1839
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/6449d6e3d9b33600080fa9d4
😎 Deploy Previewhttps://deploy-preview-6777--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to yourNetlify site settings.

@nx-cloud
Copy link

nx-cloudbot commentedMar 27, 2023
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commitd5a1839. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 32 targets

Sent with 💌 fromNxCloud.

@bradzacherbradzacher marked this pull request as ready for reviewMarch 31, 2023 23:58
@bradzacherbradzacher removed the DO NOT MERGEPRs which should not be merged yet labelMar 31, 2023
@codecov
Copy link

codecovbot commentedApr 2, 2023
edited
Loading

Codecov Report

Merging#6777 (d5a1839) intov6 (3e3b789) willincrease coverage by0.10%.
The diff coverage is20.00%.

Additional details and impacted files
@@            Coverage Diff             @@##               v6    #6777      +/-   ##==========================================+ Coverage   87.51%   87.61%   +0.10%==========================================  Files         376      375       -1       Lines       12939    12923      -16       Branches     3820     3821       +1     ==========================================- Hits        11323    11322       -1+ Misses       1231     1216      -15  Partials      385      385
FlagCoverage Δ
unittest87.61% <20.00%> (+0.10%)⬆️

Flags with carried forward coverage won't be shown.Click here to find out more.

Impacted FilesCoverage Δ
packages/utils/src/ts-eslint/Linter.ts50.00% <ø> (ø)
packages/utils/src/ts-eslint/RuleTester.ts100.00% <ø> (ø)
packages/typescript-estree/src/simple-traverse.ts71.87% <20.00%> (-4.80%)⬇️

Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Whew, what a PR! This is great - looking forward to having native dependency contraints!


import CodeBlock from '@theme/CodeBlock';

# `@typescript-eslint/rule-tester`
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldbergApr 3, 2023
edited
Loading

Choose a reason for hiding this comment

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

[Docs] Maybe we should explicitly call this out as beta/wip/similar, so folks don't see this and worry they should immediately switch to it?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

As per our chats on discord -@armano2 and I think we should push to make this a breaking change replacement in v6 - people should switch to it cos it's better in many ways and it should mostly be a 1:1 replacement.

Also switching to the package means people get theonly: boolean test prop for free, regardless of their ESLint version.

armano2 and JoshuaKGoldberg reacted with thumbs up emoji
// we intentionally import from eslint here because we need to use the same class
// that ESLint uses, not our custom override typed version
import { SourceCode } from 'eslint';
import merge from 'lodash.merge';

Choose a reason for hiding this comment

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

[Question] Should this be further forked to do the same lodash import style as our other files?

bradzacher reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I thought about that - but ESLint has this exact import for its rule tester.
So I figured that we'd just use the exact same code to avoid a duplicate install

JoshuaKGoldberg reacted with thumbs up emoji
[nodeSelector: string]: RuleFunction | undefined;
}
// Interface to merge into for anyone that wants to add more selectors
interface RuleListenerExtension {}

Choose a reason for hiding this comment

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

[Docs] This feels like something that should be mentioned in the docs page?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

That was just me adding some future-proofing in case anyone was doing something weird.
Actually don't need it - but I think it's fine to leave it undocumented for the moment.

@@ -1,24 +1,2 @@
// Note - @types/json-schema@7.0.4 added some function declarations to the type package
// If we do export *, then it will also export these function declarations.
// This will cause typescript to not scrub the require from the build, breaking anyone who doesn't have it as a dependency

Choose a reason for hiding this comment

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

[Question] Is this comment no longer necessary?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

yeah because in the olden days we didexport * from 'json-schema' and we'd export the types for functions that don't actually exist.
But now a days we can doexport type * and it'll not include the runtime bits!

Though#6963 would just be better than making this change at all TBH

@bradzacherbradzacher added this to the6.0.0 milestoneApr 27, 2023
@bradzacherbradzacher changed the titlefeat: add new package for our rule-tester forkfeat: add new packagerule-testerApr 27, 2023
@bradzacherbradzacher merged commit2ce1c1d intov6Apr 27, 2023
@bradzacherbradzacher deleted the fork-rule-tester branchApril 27, 2023 03:12
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsMay 5, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@armano2armano2armano2 left review comments

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg approved these changes

Assignees
No one assigned
Labels
enhancementNew feature or request
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

3 participants
@bradzacher@armano2@JoshuaKGoldberg

[8]ページ先頭

©2009-2025 Movatter.jp