Skip to content

Conversation

@mrazauskas
Copy link
Contributor

Resolves #18

@SimenB Here they are – e2e tests. The ideas / situations taken from readme.md. Do we need something more?

Interestingly TypeScript test does not need empty.d.ts. Did I misunderstood something?

The runJest is adapted from jest-runner-eslint.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

yay! I see there's no CI set up here - I can do that on master (and rename it to main)

@mrazauskas
Copy link
Contributor Author

yay! I see there's no CI set up here - I can do that on master (and rename it to main)

Yes, please setup CI and then I will push changes. That would be the best.

@SimenB
Copy link
Member

SimenB commented Oct 14, 2021

@mrazauskas just pushed it 🙂 I just added eslint, so you'll need a new job for Jest, but still

@mrazauskas
Copy link
Contributor Author

Perfect! Will fix everything bit later.

@mrazauskas
Copy link
Contributor Author

Done. I removed previous typescript test. Could not reproduce the situation mentioned in docs. I was looking at the linked issue too and was experimenting in different ways, but could not make it work as expected.

That’s some work around related with tsd and not this library. So I think it is fine to skip it for now.

By the way, empty.d.ts files are used in Jest type tests too. All is set up exactly how docs in this library expalin. Interestingly it seems like those empty.d.ts in Jest repo just do nothing. They can be removed and all works as expected. I will send a PR later. Just trying to explain the puzzle I was looking at.

@SimenB
Copy link
Member

SimenB commented Oct 14, 2021

By the way, empty.d.ts files are used in Jest type tests too. All is set up exactly how docs in this library expalin. Interestingly it seems like those empty.d.ts in Jest repo just do nothing. They can be removed and all works as expected. I will send a PR later. Just trying to explain the puzzle I was looking at.

ah, nice! less random files is a good thing 🙂

@SimenB
Copy link
Member

SimenB commented Oct 14, 2021

adding files breaks it :( shipping with e2e types suck tho. Lemme play with it

@SimenB
Copy link
Member

SimenB commented Oct 14, 2021

@mrazauskas this is close! Tests fail on windows for some reason (we can skip it if it's an upstream issue rather than an issue with our code)

@mrazauskas
Copy link
Contributor Author

These might be the roots of problems on Windows

// Convert absolute path to relative path
const testFile = testPath.replace(process.cwd() + '/', '');

testFile.substring(0, testFile.lastIndexOf('/')),

I will try to fix. Thanks for setting up everything. Very useful.

@SimenB
Copy link
Member

SimenB commented Oct 14, 2021

Good find! Should use path.sep and not hard code / probably? 🙂

@mrazauskas
Copy link
Contributor Author

Looked through and removed everything what seemed suspicious. Let’s see if this helps.

@SimenB
Copy link
Member

SimenB commented Oct 14, 2021

By the way, did you notice tsdjs/tsd#127? Would be really helpful to land this one.

Missed it, very exciting! Not needing the fork would be super awesome 👍

@mrazauskas
Copy link
Contributor Author

Found it. testFiles must be an array of globs, but here a relative path was passed. On Windows this throws "Could not find test files". The glob did not match any files, because slashes were wrong.

@SimenB
Copy link
Member

SimenB commented Oct 15, 2021

Closer! Seems we need to normalize the icons in the snapshot (https://github.com/facebook/jest/blob/7dd17d541bcdb4d17d96b53586949fb195294040/e2e/Utils.ts#L278-L287) and normalize the path printed by diagnostics. Jest already normalizes \ to / in paths on windows, so that would be consistent

@SimenB
Copy link
Member

SimenB commented Oct 15, 2021

so close 😀

@SimenB
Copy link
Member

SimenB commented Oct 15, 2021

Try to pass testFile here

`${testFile}:${test.line}:${test.column} - ${test.severity} - ${test.message}`
though slash

@mrazauskas
Copy link
Contributor Author

A.. Got it already. Just a minute (;

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

🎉

@SimenB SimenB merged commit 0a11225 into jest-community:main Oct 15, 2021
@mrazauskas
Copy link
Contributor Author

Yes! Thanks. It was fun.

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.

Add tests

2 participants