Skip to content

transformedTests#114

Open
bogeeee wants to merge 7 commits intotypescript-rtti:mainfrom
bogeeee:TransformedTestsPR
Open

transformedTests#114
bogeeee wants to merge 7 commits intotypescript-rtti:mainfrom
bogeeee:TransformedTestsPR

Conversation

@bogeeee
Copy link
Copy Markdown
Contributor

@bogeeee bogeeee commented Nov 28, 2023

See readme.

Tests, for which the code has type information transformed in. This way, it's much easier to write mass tests for the validator. Just the project setup with some basic examples
i.e.

class A { // This class will be enhanced with type info
    foo() { return true ? 123 : 'foo'; }
}
expect( reflect(A).getMethod('foo').returnType.isUnion() ).to.be.true

@bogeeee bogeeee mentioned this pull request Nov 28, 2023
Open
rezonant
rezonant requested changes Jan 28, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@rezonant rezonant left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Definitely up for more discussions around the TODOs you've got in here, but I like the general idea you're going for. What do you think of calling the directory integration-tests instead of transformedTests? The code already uses dash-casing for its filenames, and I think what these tests really do is test the "whole shebang", but in a faster and easier to work with way than the corpus test that builds external projects.

EDIT: See my comment at the bottom of the review about the ts-patch/TS5 stuff

That's all in addition to, as you said, making it easier for people to contribute tests, both for validation purposes and (I hope!) for submitting with their PRs and bug reports.

Comment thread transformedTests/package.json Outdated
Comment thread transformedTests/tsconfig.json
"declaration": false,
"plugins": [
{
"transform": "typescript-rtti/dist/transformer"
Copy link
Copy Markdown
Member

@rezonant rezonant Jan 28, 2024

Choose a reason for hiding this comment

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

set this to "../dist/transformer"

Copy link
Copy Markdown
Contributor Author

@bogeeee bogeeee Feb 5, 2024

Choose a reason for hiding this comment

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

Works, as is, after the change in package.json

Comment thread transformedTests/readme.md Outdated
Comment thread transformedTests/readme.md Outdated
@rezonant
Copy link
Copy Markdown
Member

rezonant commented Jan 28, 2024 •
edited
Loading

Oh wait I see, it's because ts-patch finds TS 4.9 in the parent :-S.

It really shouldn't. Even if I add TS5 into the subfolder, it still complains. If I install TS5 in the root it works fine.

That being said, I think this PR should target 4.9 unless/until the main project upgrades to 5.x. Note that while the transformer supports up to TS 5.1, I left the main devdependency at TS 4.9 since it is the minimum supported, the idea being to help avoid breaking compatibility inadvertently.

rezonant
rezonant requested changes Jan 28, 2024
View reviewed changes
Comment thread transformedTests/package.json Outdated
@bogeeee
Copy link
Copy Markdown
Contributor Author

bogeeee commented Feb 5, 2024

That being said, I think this PR should target 4.9 unless/until the main project upgrades to 5.x.

Done so.

@bogeeee bogeeee closed this Feb 5, 2024
@bogeeee bogeeee reopened this Feb 5, 2024
@bogeeee bogeeee requested a review from rezonant February 5, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rezonant rezonant Awaiting requested review from rezonant

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bogeeee @rezonant