Meta issue: Plan of action

[johnnyreilly]

Things to do:

• Cut down the comparison test matrix to only run only against TypeScript 2.0
• Make comparison test pack more resilient / less flaky
• Get integration tests passing for TypeScript 2.0
• Publish ts-loader v0.9
• Create a new integration test pack that executes javascript created by ts-loader against a headless browser
• Update CONTRIBUTING.md to cover both comparison test pack and execution test pack; making clear the differences, usage and how to author
• Get ts-loader continuous integration running against TypeScript versions 1.6, 1.7, 1.8, 2.0 and TypeScript@next
• Port appropriate tests across from comparison test pack to execution test pack (i.e. ones that don't test for compiler errors / source maps etc). These seem good candidates for migration:
  • jsx
  • jsxPreserve (rename to babel-jsxPreserve?)
  • replacement
• Add new tests to the execution test pack. These seem like scenarios it would be good to test:
  would like to add
  • test demonstrating moduleResolution "node" (already covered by nodeResolution?)
  • test demonstrating allowSyntheticDefaultImports
  • test demonstrating large project (multiple imports); like large in comparison tests but more depth of files; interdependencies etc.
  • test demonstrating react-addons-test-utils?

Okay here's the deal; I've been using ts-loader for a long time but my contributions up until now have mostly been documentation. Fixing of tests etc. As the commit history shows this is @jbrantly's baby and kudos to him.

He's not been able to contribute much of late and since he's the main person who's worked on ts-loader not much has happened for a while; the code is a bit stale. As I'm a member of TypeStrong I'm going to have a go at improving the state of the project. I'm going to do this as carefully as I can. This issue is intended as a meta issue to make it visible what I'm plannning to do / doing.

My immediate goal is to get a newer version of ts-loader built and shipped. Essentially all the bug fixes / tweaks since the last release should ship. This includes:

• making ts-loader compatible with node v6 a4f8353
• fixing the declaration issue 3bb0fec
• declarations update independent of compiler.watchFileSystem ae824b2
• various optimisations aff605e

What needs to happen to get us to this point:

The integration tests are completely failing.

I need to regenerate the test data for TypeScript v2.0. This in itself may be problematic.

The integration test pack. It's super brittle. As I understand it, it's essentially comparing a known state of success regarding files generated by webpack with the files generated from running right now. As each dependency is updated it invariably "breaks" a test. Which doesn't necessarily represent a breakage in ts-loader. Those changed files don't necessarily mean that the newly generated code isn't working. It is generally working but different. But the test fails all the same. This leads to false negatives (many) and doesn't incentivise people to contribute to ts-loader.

In addition, when following the instructions to run tests / regenerate test data I've started to notice the test pack failing midway with garbage collection errors. I'm no Node expert and so that's a tricky one.

Long story short; I'd rather the integration test pack was a very different beast; significantly simpler and probably not based on file comparisons. But I don't have the time to create a new one so I'm going to do my best with what's in place now.

Cut down the test matrix to a manageable size

Because maintaining multiple versions of the integration tests has not been happening (and I'm not proposing to do it myself) I'm intending to reduce the test matrix from this:

• TYPESCRIPT=typescript@1.6.2
• TYPESCRIPT=typescript@1.7.5
• TYPESCRIPT=typescript@1.8.0
• TYPESCRIPT=typescript@2.0.3
• TYPESCRIPT=typescript@next

to this:

• TYPESCRIPT=typescript@2.0.3

i.e Only have tests for the current version of the TypeScript. That's more maintainable. I won't delete the existing test data (apart from the never to be TypeScript 1.9) in case anyone wants to pick up the baton later. I bet they don't.

How to publish?

I don't have npm publish rights for ts-loader. Fortunately both @jbrantly and @blakeembrey do - and hopefully one of them will either be able to help out with a publish or let me have the requisite rights to do it.

I can't promise this is all going to work; I've got a limited amount of spare time I'm afraid. Whatever happens it's going to take me a little while. But I'm going to see where I can take this. Best foot forward! Please bear with me...

[jbrantly]

@johnnyreilly I've added you as an owner on npm so you can publish now. You're the man for picking up this torch yourself. I do have a major improvement to the unit tests that I worked on like 6 months ago that I can try to get at some point which greatly improves the stability of them (hopefully no more random failures)

[johnnyreilly]

Awesome thanks for that! 👍 Any help you can provide will always be gratefully received - particularly on the unit tests.

In the meantime I'll feel my way forwards and see if we can get something out. Well done on all the work you've done on ts-loader so far. I use it every day and it rocks!

[blakeembrey]

For the unit tests, can we simplify it and have a "generate" step and a separate test step? Basically then we're just comparing snapshots, makes life a ton easier and validating would just be visual. That way when something does break, it'll be a quick step to re-generate them, compare the git diff and commit. It's what I've been doing on a lot of other projects like this anyway 😄

For the separate versions, perhaps having multiple test run in the context of node_modules installed at different levels so that all versions can be installed and tested together (and most importantly, generated together). The only tricky thing would be if there's any Windows vs Unix output differences, so some normalization might need to occur. E.g. /test/fixtures/typescript@1.8.0/node_modules for each TypeScript version and just link them or something for each test version run locally? I'd be down to make it work, but have been too afraid to touch everything in the current state.

[jbrantly]

For the unit tests, can we simplify it and have a "generate" step and a separate test step?

I think that's already in place. See https://github.com/TypeStrong/ts-loader/blob/master/CONTRIBUTING.md#regenerating-test-data It works exactly like you said, you just compare the changes in git and commit.

[blakeembrey]

Thanks. I noticed looking through the other PRs from @johnnyreilly - you can tell I was avoiding it now 😄 I did spend a day refactoring the code at one point, but aborted because of the tests (it was probably more than just regenerating then). Does it also handle multiple TypeScript releases to regenerate fixtures?

[johnnyreilly]

The only tricky thing would be if there's any Windows vs Unix output differences

I think this is already handled by this.

[johnnyreilly]

For the unit tests, can we simplify it and have a "generate" step and a separate test step?

I think that's already in place. See https://github.com/TypeStrong/ts-loader/blob/master/CONTRIBUTING.md#regenerating-test-data It works exactly like you said, you just compare the changes in git and commit.

Yeah I think this is already in place; unless you meant something I didn't get? The tricky thing is that node rolls over and dies during the regeneration at the moment. (Something to do with garbage collection but not sure what) I actually considering generating the files in batches initially just to get up and running..

[johnnyreilly]

Does it also handle multiple TypeScript releases to regenerate fixtures?

Not that I've spotted... Just uses current dependency. So if you want to regenerate for all versions then you have manually tweak the installed version of TypeScript until you have all the versions you care about. Icky.

[jbrantly]

Does it also handle multiple TypeScript releases to regenerate fixtures?

I can confirm it does not. A PITA. You have to npm install typescript@v && npm test ...

[johnnyreilly]

Okay - here's where we are:

• I've got rid of the 1.9 test data
• removed typescript@next from the build matrix
• upped ts-loader to typescript 2.0
• regenerated the test data (had to do it in batches because of garbage collection issues)

If you look at the latest build you'll see that with TypeScript 2 the integration test runner rolls over and dies for the self same garbage collection issues when running the tests.

Does anyone have any insights on what might cause this / how I could work around this?

It seems to be a node problem (well maybe) - related issues:
webpack/webpack#1875
nodejs/node#1741
karma-runner/karma#1868

ts-loader seems to work just fine - tempted to publish a new version regardless of integration tests once I've done some independent testing on a couple of my own projects.

[johnnyreilly]

@basarat - have you seen this node weirdness at all? I've been trying out various workarounds for the garbage collection issue but nothing's working. It feels like I'm spitting in the wind... Casting around for anyone who might have suggestions. It looks like the TypeScript compiler is doing a lot more as of ts 2.0 but I'm not too sure where to begin here. Have you seen any oddness as you've been working on alm tool? Forgive the question but I'm kinda out of inspiration...

[basarat]

Have you seen any oddness as you've been working on alm tool?

No I have not had memory issues with TypeScript @ next. That said:

Next is a bit crashy too, e.g. alm-tools/alm#208 and alm-tools/alm#211 ALM is now equipped to deal with ts crashing. In both cases ts-loader crashes too and takes down webpack with it :)

Also not sure if its my bug or TypeScript but I am also now getting this : microsoft/TypeScript#11377

I have always been on next and haven't looked back at 2.0 so not much help there. Thanks for including me still 🌹

[johnnyreilly]

Thanks for that @basarat - if anyone was worth asking it was definitely you 😄

I'll raise an issue with the TypeScript team in case there's anything they can suggest. Given other projects are experiencing similar issues I kind of suspect it's not actually their issue; probably the compiler is just doing more than it used to and revealing an already existing issue either in node or WebPack. I don't really know.

[johnnyreilly]

Okay - I was able to workaround the GC issue by kicking off each test in it's own child process. I've also got the tests passing as of: 8d64847

Flaky tests have been marked as just that and are no longer failing the build. 😄 The reasons they're failing the build seem to be insignificant. I've also done independent testing on 2 projects and ts-loader seems to be a going concern.

Long story short: I think I'm ready to get a release out. Watch this space....

[johnnyreilly]

0.9.0 has shipped!

[johnnyreilly]

Did some work on the test pack to ignore what seem to be spurious differences in output*.txt files, namely:

• '/' and '' differences between *nix and Windows
• ' [built]' is not always present - now ignored when not

Only 2 tests are now considered flaky as a result

[johnnyreilly]

I know I said:

I'd rather the integration test pack was a very different beast; significantly simpler and probably not based on file comparisons. But I don't have the time to create a new one...

So it turns out I lied: #305

[johnnyreilly]

So there are now 2 test packs; the comparison test pack (of long standing) and the execution test pack (brand new). The execution test pack executes karma tests using ts-loader and phantomjs (any browser would do). The advantage of the new test pack is it doesn't rely on maintaining expected outputs. Theoretically you can just use any version of TypeScript and give the test pack a whirl.

I've updated the contributing.md file to cover both test packs.

[johnnyreilly]

Oh and ts-loader is now running against ts versions 1.6 to 2.0 as well as TypeScript@next. The comparison test pack only runs against 2.0. I plan to add many more tests than the present 4 to the execution test pack. I think I can use comparison tests as a basis for many of these; I've ported 2 across already.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

Closing as stale. Please reopen if you'd like to work on this further.