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.

Sign up for GitHub

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

Jump to bottom

Enable typescript for jest tests #3440

Merged
merged 1 commit into from
Feb 28, 2018
Merged

Enable typescript for jest tests #3440

merged 1 commit into from
Feb 28, 2018

Conversation

karelhala
Copy link
Contributor

@karelhala karelhala commented Feb 21, 2018 •
edited
Loading

Adds support for typescript files for Jest

As we support typescript for writing packs we should support jest to work with TS files as well. This PR enables such thing with adding ts-jest package and changing jest config to work with both TS and JS files.

  • JS files should be transformed by babel-jest
  • TS files should be transformed by ts-jest
  • Mock old JS files to test process

@karelhala
Copy link
Contributor Author

@miq-bot add_label enhancement, build
@miq-bot assign @himdel
@ohadlevy FYI

@miq-bot
Copy link
Member

miq-bot commented Feb 21, 2018

@karelhala Cannot apply the following label because they are not recognized: build

@karelhala karelhala force-pushed the enableTsForJest branch 2 times, most recently from 8ff9176 to 9c96429 Compare February 21, 2018 14:52
@karelhala karelhala mentioned this pull request Feb 21, 2018
Merged
3 tasks
@karelhala
Copy link
Contributor Author

Restarting tests

@karelhala karelhala closed this Feb 21, 2018
@karelhala karelhala reopened this Feb 21, 2018
himdel
himdel reviewed Feb 22, 2018
View reviewed changes
config/jest.setup.js Outdated
@@ -0,0 +1,10 @@
window.angular = require('angular');
window.Rx = require('rxjs');
window.$ = require('../vendor/assets/bower/bower_components/jquery/dist/jquery');
Copy link
Contributor

Choose a reason for hiding this comment

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

require('jquery') now :) (#3450 got merged)

himdel
himdel reviewed Feb 22, 2018
View reviewed changes
jest.config.js Outdated
@@ -5,5 +5,14 @@ module.exports = {
__testing__: true
},
roots: ['app/javascript'],
testMatch: ['**/*.test.js']
setupFiles: ['./config/jest.setup.js'],
testRegex: '(/__tests__/.*|(\\.|/)(test))\\.(js?|ts?)$',
Copy link
Contributor

@himdel himdel Feb 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Would it make sense to name the dir specs/ as with old js? Or is this a new convention to use __tests__/?
(I'm fine with either.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we may want to support *.spec.js since that's what SUI does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can either put it into new folder __tests__ or leave them in any folder, that is jus optional. However you have to name your file ${whatever}.test.[js|ts] this is because there might be clash between jasmine old tests and new one's written in jest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or no, there is root directory, I'll add option to use spec or test as part of file.

@karelhala karelhala closed this Feb 22, 2018
@karelhala karelhala reopened this Feb 22, 2018
himdel
himdel reviewed Feb 27, 2018
View reviewed changes
jest.config.js Outdated
@@ -5,5 +5,14 @@ module.exports = {
__testing__: true
},
roots: ['app/javascript'],
testMatch: ['**/*.test.js']
setupFiles: ['./config/jest.setup.js'],
testRegex: '(/__tests__/.*|(\\.|_|/)(test|spec))\\.(js?|ts?)$',
Copy link
Contributor

Choose a reason for hiding this comment

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

\.(js?|ts?)$ will match .j and .js but not .jsx ;)

I guess you wanted \.(jsx?|tsx?)$?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are totally right.

@miq-bot
Copy link
Member

miq-bot commented Feb 28, 2018

Checked commit karelhala@2565873 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🍰

@himdel himdel mentioned this pull request Feb 28, 2018
Closed
@himdel himdel modified the milestone: Sprint 81 Ending Mar 12, 2018 Feb 28, 2018
@himdel
Copy link
Contributor

himdel commented Feb 28, 2018

LGTM, waiting for green :).

Tested the regex, looks like it currently matches only

$ find app/ -type f  | perl -ne 'print if m#(/__tests__/.*|(\.|_|/)(test|spec))\.(jsx?|tsx?)$#' 
app/javascript/react/compontentRegistry.test.js

.. so perfect :)

(It would also match ./config/webpack/test.js which is perhaps less than ideal, but .. not a problem, since we don't run tests from config/ :).)

@himdel himdel merged commit 3a11da8 into ManageIQ:master Feb 28, 2018
@himdel himdel added this to the Sprint 81 Ending Mar 12, 2018 milestone Feb 28, 2018
@dclarizio dclarizio added the test label Mar 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@himdel himdel himdel left review comments

Assignees

@himdel himdel

Labels
enhancement test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@karelhala @miq-bot @himdel @dclarizio