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

[eslint-plugin] fix ts-modules-only-named invalid test cases #30924

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

jeremymeng
Copy link
Contributor

There's some change in typescript-eslint that breaks some eslint test cases.
Previously when running tests, in a rule's create() method, we get a relative
path for context.filename, and when we pass a setting of { main: "src/test.ts" } in rule tester option, the two were considered the same file,
therefore the following condition check failed, and we continue to report error
for default exports.

if (relative(normalize(context.filename), normalize(context.settings.main as string)) !== "") {

After the upgrade, context.filename now contains the full path, the file
src/test.ts in our test fixture has a full path of
/.../common/tools/eslint-plugin-azure-sdk/tests/fixture/src/test.ts. It is not
considered the same file as the passed settings implies
/.../common/tools/eslint-plugin-azure-sdk/src/test.ts when we run the tests.
So we return earlier instead of going to check for default exports.

This is not an issue in real scenario where we have settings of { main: "src/index.ts" }, which would match the full path of the file we are linting
when we run eslint from a package directory.

This PR fixes the test by passing tests/fixture/src/test.ts in the settings so
that it matches the full path of context.filename we get in create().

There's some change in `typescript-eslint` that breaks some eslint test cases.
Previously when running tests, in a rule's `create()` method, we get a relative
path for `context.filename`, and when we pass a setting of `{ main:
"src/test.ts" }` in rule tester option, the two were considered the same file,
therefore the following condition check failed, and we continue to report error
for default exports.

https://github.com/Azure/azure-sdk-for-js/blob/6dd8b582e2f5a3257befab368bab0e64499f4d57/common/tools/eslint-plugin-azure-sdk/src/rules/ts-modules-only-named.ts#L31

After the upgrade, `context.filename` now contains the full path, the file
`src/test.ts` in our test fixture has a full path of
`/.../common/tools/eslint-plugin-azure-sdk/tests/fixture/src/test.ts`. It is not
considered the same file as the passed settings implies
`/.../common/tools/eslint-plugin-azure-sdk/src/test.ts` when we run the tests.
So we return earlier instead of going to check for default exports.

This is not an issue in real scenario where we have settings of `{ main:
"src/index.ts" }`, which would match the full path of the file we are linting
when we run `eslint` from a package directory.

This PR fixes the test by passing `tests/fixture/src/test.ts` in the settings so
that it matches the full path of `context.filename` we get in `create()`.
@jeremymeng jeremymeng merged commit b7e1fd2 into Azure:main Aug 28, 2024
17 checks passed
@jeremymeng jeremymeng deleted the eslint/fix-tests branch August 28, 2024 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants