Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

feat(tests): make ts-jest compile tests #445

Merged
merged 4 commits into from
Nov 15, 2018

Conversation

bmdalex
Copy link
Collaborator

@bmdalex bmdalex commented Nov 7, 2018

feat(tests): make ts-jest compile tests

Problem:

We are not including test/ folder in our typescript compiler options so nothing under tests is compiled by TypeScript.

@layershifter did some research and found out the following:

We're using a Gulp's task to run jest and our watchers for JSON info files, #task. In the Jest's config we will see, that we're using ts-jest. So the main question why ts-jest compiles and runs the code silently? 👎

Solution:

The version bump was enough to get it working correctly:

"jest": "^23.6.0",
"jest-axe": "^3.1.0",
"ts-jest": "^23.10.4",

image

Proposal:

I talked to @smykhailov about this as well and he suggested that we should not rely on all these custom build scripts and tools (such as gulp / grunt) for our build. Reasons:

  • we end up creating build scripts everywhere that are hard to understand and maintain.
  • custom build scripts can introduce lots of errors and bugs as this one;
  • custom build scripts contain duplicated work (webpack accomplishes out of the script functionality and we only need to configure it).

We should leverage more the webpack functionality for setting up our build. This might be a good time to do this since we are just kicking off an effort for creating a Stardust monorepo #433

Do we have any issue related to our build system approach and tooling? I can create an issue and copy these findings there.

What is your take on this, guys?
@levithomason
@kuzhelov
@miroslavstastny
@layershifter
@mnajdova
@alinais
@smykhailov

@bmdalex bmdalex added ⚙️ enhancement New feature or request question Further information is requested, concerns that require additional thought are raised RFC 🚧 WIP labels Nov 7, 2018
@bmdalex bmdalex added the needs author changes Author needs to implement changes before merge label Nov 7, 2018
@bmdalex
Copy link
Collaborator Author

bmdalex commented Nov 8, 2018

@layershifter here is the RFC I had for the test folder not being compiled by typescript; we discussed it at standup today; can you provide your input in the comments?

@layershifter
Copy link
Member

I'm fully agree about crazy tooling, we should massively simplify them in the future.

build/gulp/tasks/dist.ts

You have updated a task in this file, in fact it will just include tests to our dist build. It's completely weird thing.

build/tsconfig.commonjs.json

We don't need to update this config because ts-jest uses the default config from root, test directory is included there.


We're using a Gulp's task to run jest and our watchers for JSON info files, #task. In the Jest's config we will see, that we're using ts-jest. So the main question why ts-jest compiles and runs the code silently? 👎

I didn't dig the issue deeply, but the version bump was enough to get it working correctly:

    "jest": "^23.6.0",
    "jest-axe": "^3.1.0",
    "ts-jest": "^23.10.4",

image

@bmdalex
Copy link
Collaborator Author

bmdalex commented Nov 8, 2018

build/gulp/tasks/dist.ts
You have updated a task in this file, in fact it will just include tests to our dist build. It's completely weird thing.

Oh yes, fully agree I did something very weird there, I was just trying to make a point that there's something strange about tests compilation by hacking stuff. Probably I should have created an issue and not an invalid PR.

I didn't dig the issue deeply, but the version bump was enough to get it working correctly:
very nice 💯 , I guess there are plenty of errors to fix. Feel free to open a PR for the version bump and someone can pick it up and fix the errors. I'll then close this one and move all the findings to the new PR. Thanks a lot, @layershifter !

@bmdalex bmdalex force-pushed the feat/build-add-test-to-ts-compile branch 2 times, most recently from 5bb7c31 to 2a51b7e Compare November 9, 2018 16:45
@codecov
Copy link

codecov bot commented Nov 9, 2018

Codecov Report

Merging #445 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #445      +/-   ##
==========================================
- Coverage   88.47%   88.38%   -0.09%     
==========================================
  Files          41       41              
  Lines        1432     1421      -11     
  Branches      181      206      +25     
==========================================
- Hits         1267     1256      -11     
  Misses        161      161              
  Partials        4        4
Impacted Files Coverage Δ
src/components/Input/Input.tsx 100% <ø> (ø) ⬆️
src/index.ts 100% <0%> (ø) ⬆️
src/components/Popup/positioningHelper.ts 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b0d98f...bd5af9d. Read the comment docs.

@bmdalex bmdalex changed the title [RFC]: feat(build): add test/ folder to ts compiler feat(build): add test/ folder to ts compiler Nov 9, 2018
@bmdalex bmdalex changed the title feat(build): add test/ folder to ts compiler feat(tests): make ts-jest compile tests Nov 9, 2018
@bmdalex bmdalex force-pushed the feat/build-add-test-to-ts-compile branch from 2a51b7e to 9c08bfc Compare November 9, 2018 17:00
@bmdalex bmdalex changed the title feat(tests): make ts-jest compile tests feat(tests): make ts-jest compile tests Nov 9, 2018
@bmdalex bmdalex added 🚀 ready for review and removed RFC ⚙️ enhancement New feature or request needs author changes Author needs to implement changes before merge question Further information is requested, concerns that require additional thought are raised 🚧 WIP labels Nov 9, 2018
@bmdalex bmdalex force-pushed the feat/build-add-test-to-ts-compile branch 2 times, most recently from 1477df6 to 8bdfa07 Compare November 13, 2018 09:22
@bmdalex bmdalex self-assigned this Nov 13, 2018
@bmdalex bmdalex force-pushed the feat/build-add-test-to-ts-compile branch from 8bdfa07 to 9f730c6 Compare November 14, 2018 14:25
@bmdalex
Copy link
Collaborator Author

bmdalex commented Nov 14, 2018

@layershifter addressed all comments, but we need an approved reviewer; let's bring this up with @levithomason

@bmdalex bmdalex force-pushed the feat/build-add-test-to-ts-compile branch from 9f730c6 to a80dd9e Compare November 15, 2018 13:42
Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

I don't happy with all changes in this PR, but I understand the reasons. In the future we should address tests simplification.

Green light from me 💚

@bmdalex bmdalex force-pushed the feat/build-add-test-to-ts-compile branch from 735541f to bd5af9d Compare November 15, 2018 14:48
@bmdalex bmdalex merged commit 267b46a into master Nov 15, 2018
@bmdalex bmdalex deleted the feat/build-add-test-to-ts-compile branch November 15, 2018 15:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants