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

test: enable suppressTypeErrors on all integration tests #1549

Conversation

rubiesonthesky
Copy link
Collaborator

@rubiesonthesky rubiesonthesky commented Apr 7, 2024

PR Checklist

Overview

Enables suppressTypeErrors on all integration tests to track potential type errors on generated code.

Also, makes sure that we do not add absolute module path to the ts ignore comment as it would be different when run on different machines.

@rubiesonthesky rubiesonthesky added the status: blocked Waiting for something else to be resolved 🙅 label May 5, 2024
@rubiesonthesky rubiesonthesky force-pushed the enable-suppress-type-errors-on-integration-tests branch from 3a66f23 to 56e4f5d Compare December 7, 2024 15:46
@rubiesonthesky rubiesonthesky force-pushed the enable-suppress-type-errors-on-integration-tests branch from 56e4f5d to 1745a1d Compare December 7, 2024 15:47
Copy link

codecov bot commented Dec 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.50%. Comparing base (a669242) to head (aae2471).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1549      +/-   ##
==========================================
+ Coverage   75.35%   75.50%   +0.15%     
==========================================
  Files         177      177              
  Lines        7566     7585      +19     
  Branches     1069     1077       +8     
==========================================
+ Hits         5701     5727      +26     
+ Misses       1860     1853       -7     
  Partials        5        5              
Flag Coverage Δ
mutation 65.26% <100.00%> (+0.17%) ⬆️
unit 17.12% <5.00%> (-0.03%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rubiesonthesky rubiesonthesky removed the status: blocked Waiting for something else to be resolved 🙅 label Dec 7, 2024
@rubiesonthesky rubiesonthesky marked this pull request as ready for review December 7, 2024 15:53
@rubiesonthesky
Copy link
Collaborator Author

@JoshuaKGoldberg I hope that this one is more straightforward to review :)

@rubiesonthesky
Copy link
Collaborator Author

@JoshuaKGoldberg friendly ping! Could you check if this makes sense to you? I think it will help in the future changes to see if the code in tests is correct or not :)

Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

OK! Thanks for the ping, and sorry this took so long. Kind of a behemoth to review 😅 but a very good step for comprehensive tests. Thank you for pushing it forward!

@@ -113,6 +116,7 @@
};

const returnsBigintOrNumber = (): number | bigint => {
// @ts-expect-error -- TODO: BigInt literals are not available when targeting lower than ES2020.
Copy link
Owner

Choose a reason for hiding this comment

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

[Testing] Let's switch the test to use ESNext. Testing for this error isn't the point of the test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's currently set to "ES2020", and it doesn't change the error if I set it to "ESNext".

return <div />;
}
}

const renderSinglePropClassDeclaration = () => (
// @ts-expect-error -- TODO: Cannot use JSX unless the '--jsx' flag is provided.
Copy link
Owner

Choose a reason for hiding this comment

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

[Testing] Tests with .tsx files should enable the jsx flag.

import React from "react";
// @ts-expect-error -- TODO: This module can only be default-imported using the 'esModuleInterop' flag
Copy link
Owner

Choose a reason for hiding this comment

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

[Testing] This and other tests should have esModuleInterop enabled.

@@ -0,0 +1,6 @@
// @ts-expect-error -- TODO: This module can only be default-imported using the 'esModuleInterop' flag
import ts from "typescript";
// @ts-expect-error -- TODO: Cannot find module './typestat.json'. Consider using '--resolveJsonModule' to import module with '.json' extension.
Copy link
Owner

Choose a reason for hiding this comment

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

[Testing] The test should have resolveJsonModule so this gets imported nicely.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It has it... But the compiler options reading is broken. My other PR fixes it, but I'd like to have this one first so I can show that it fixes these issues :)

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author Needs an action taken by the original poster label Jan 3, 2025
@JoshuaKGoldberg JoshuaKGoldberg changed the title fix(tests): enable suppressTypeErrors on all integration tests test: enable suppressTypeErrors on all integration tests Jan 3, 2025
@rubiesonthesky
Copy link
Collaborator Author

I'm not able to fix the ts errors by changing their tsconfig settings, because those settings are already there! The reading of tsconfig files is broken and this is a way to show it :D

This PR #1542 is supposed to fix it

So if it's okay let's leave the ts errors be present in the tests for now, and let's see how many of them will be fixed in that PR. Or there is #2053 which is combination of this PR and 1542. But I think it's better to go step by step.

Or what do you think? :)

@JoshuaKGoldberg
Copy link
Owner

OH! That totally makes sense, I missed that. Agreed - thank you :)

Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

You are absolutely right. 🚢 !~

@JoshuaKGoldberg JoshuaKGoldberg merged commit aa1d480 into JoshuaKGoldberg:main Jan 3, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for author Needs an action taken by the original poster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🧪 Tests: Enable suppressTypeErrors on all integration tests
2 participants