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

Add typescript/ts/tsx extension as a playground languages #1543

Closed
wants to merge 5 commits into from

Conversation

mitsuruog
Copy link
Contributor

Resolve:

I added typescript, ts, tsx as a playground language.

It might make breaking current behavior.
it's up to you whichever it goes to the current version or next major version.

@codecov
Copy link

codecov bot commented Feb 27, 2020

Codecov Report

Merging #1543 into master will not change coverage.
The diff coverage is 100%.

Impacted Files Coverage Δ
src/loaders/utils/chunkify.ts 91.66% <100%> (ø) ⬆️

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request! I've left couple of minor comments, and we can release it with 11.0, which is kinda ready in a branch now.

`;

const actual = chunkify(markdown);
expect(actual).toMatchSnapshot();
Copy link
Member

Choose a reason for hiding this comment

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

Let's avoid a snapshot here: it's hard to see if it's correct or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so should I write the test something like this?

const expected = [
  // expected result here
];
const actual = chunkify(markdown);
expect(actual).toEqual(expected);

Copy link
Member

Choose a reason for hiding this comment

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

Here you can check all of the chunks, except the first one, have the code type — that's the intent of the test. With a snapshot, it's impossible to tell what was the intent and whether it's correct.

src/loaders/utils/__tests__/chunkify.spec.ts Outdated Show resolved Hide resolved
Co-Authored-By: Artem Sapegin <artem@sapegin.ru>
@mitsuruog
Copy link
Contributor Author

@sapegin I updated my test code. please review me again.

@mitsuruog
Copy link
Contributor Author

@sapegin I updated my test code. please review me again.

const actual = chunkify(markdown);
expect(actual).toMatchSnapshot();
expect(actual).toEqual(expected);
Copy link
Member

Choose a reason for hiding this comment

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

Why not something like, so the test intent is actually clear?

Suggested change
expect(actual).toEqual(expected);
expect(actual.slice(1).every(chunk => chunk.type === 'code')).toBe(true)

Not it's exactly the same as it was with a snapshot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't copy as is what the snapshot did. I thought the current tests also can test for not playground extensions.
but I would like to follow your suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't say you've copied but essentially it does the same.

@mitsuruog
Copy link
Contributor Author

@sapegin I updated my test code. please review me again.

@sapegin
Copy link
Member

sapegin commented Feb 28, 2020

The idea is not to have a snapshot. Please have a look at my suggestion in my previous comment.

@mitsuruog
Copy link
Contributor Author

I was misunderstood what you said. but now it is clear. thank you.
Previous tests compared objects with each other. That didn't tell exactly which point was testing.
but now I can see that your suggestion clearly checks only the code part.

@sapegin thank you for many times review.

@sapegin sapegin changed the base branch from master to 11 February 28, 2020 13:07
@sapegin sapegin changed the base branch from 11 to master February 28, 2020 13:08
Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

Thanks!

@sapegin sapegin changed the base branch from master to 11 February 28, 2020 13:40
@sapegin sapegin changed the base branch from 11 to master February 28, 2020 13:40
@sapegin
Copy link
Member

sapegin commented Feb 28, 2020

Merged manually to 11 branch: 5975c7c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants