-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Mock out CSS files for jest runner #13315
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one suggestion and one minor nitpick about file naming!
src/jest/config.json
Outdated
@@ -13,7 +13,8 @@ | |||
"!ui_framework/components/**/*/index.js" | |||
], | |||
"moduleNameMapper": { | |||
"^ui_framework/components": "<rootDir>/ui_framework/components" | |||
"^ui_framework/components": "<rootDir>/ui_framework/components", | |||
"\\.(css|less|scss)$": "<rootDir>/src/jest/mockCssFiles.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to borrow something similar from https://github.com/elastic/kibana/pull/13172/files#diff-68998b8839f75d151a840687485895a5R16?
"\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$": "<rootDir>/ui_framework/src/test/file_mock.js"
It encompasses more files, which will be useful once we introduce SVGs (and I guess MP3s?) 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we have a visualization for audio waveforms ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am of the opinion that we should add extensions as they become necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason @tylersmalley?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Start simple" would be my reason. If this list was to exactly match the webpack loader list I would expect it to be referenced in a DRY fashion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we can start simple and add as time goes on. I'm just not sure I see a problem with having a comprehensive list up front. These files will never actually be useful within a Jest test, right? So adding them all now will solve all problems we'll encounter with them, for everyone, forever.
On the other hand, if we kept this list simple to start with, then everyone who adds a new static file will go through the same painful process (which I went through):
- They'll get an obscure webpack error about a missing loader
- Do a Google search for 20 mins
- Figure out it's a problem with static files
- Probably end up just pasting in the comprehensive list of static files, since it's the recommendation on the Jest website (see link in PR description)
All of that, for what gain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Jest docs seem to suggests to make separate mocks for styles than other static assets:
{
"moduleNameMapper": {
"\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$": "<rootDir>/__mocks__/fileMock.js",
"\\.(css|less)$": "<rootDir>/__mocks__/styleMock.js"
}
}
This might be a good idea since with styles we don't care about the returned value:
import 'mystyles.css'
whereas with images (and other static assets) we do:
import cat from 'cat.gif'
I can add the mocks for styles and assets in general, or just add the mock for the styles. Up to you.
src/jest/mockCssFiles.js
Outdated
@@ -0,0 +1 @@ | |||
module.exports = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of this file should be snake_cased, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the convention I'll rename :)
177b8da
to
72331f4
Compare
@cjcenizal @tylersmalley I've fixed the feedback. I've added the static file types as a seperate commit. Let me know if I should remove that again, or if you want it to stay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Thanks for doing this!
…13479) * Rename tasks to createComponent and documentComponent. * Reference correct src paths in README. * Add children and className to templates' propTypes. * Add default folder name for page demo. * Add suffix to sandbox routes. * Specify testPathIgnorePatterns more clearly. * Rename component.test.js to test.js so that Jenkins won't try to run it. * Update npm scripts to depend on local yo dependency, not global. * Add ui_framework/src to copy task. * Simplify npm scripts and remove requirement for installing Yeoman from README. * Mock out static files when running in Jest (#13315) * Add services to moduleNameMapper in jest config.
…lastic#13479) * Rename tasks to createComponent and documentComponent. * Reference correct src paths in README. * Add children and className to templates' propTypes. * Add default folder name for page demo. * Add suffix to sandbox routes. * Specify testPathIgnorePatterns more clearly. * Rename component.test.js to test.js so that Jenkins won't try to run it. * Update npm scripts to depend on local yo dependency, not global. * Add ui_framework/src to copy task. * Simplify npm scripts and remove requirement for installing Yeoman from README. * Mock out static files when running in Jest (elastic#13315) * Add services to moduleNameMapper in jest config.
…lastic#13479) * Rename tasks to createComponent and documentComponent. * Reference correct src paths in README. * Add children and className to templates' propTypes. * Add default folder name for page demo. * Add suffix to sandbox routes. * Specify testPathIgnorePatterns more clearly. * Rename component.test.js to test.js so that Jenkins won't try to run it. * Update npm scripts to depend on local yo dependency, not global. * Add ui_framework/src to copy task. * Simplify npm scripts and remove requirement for installing Yeoman from README. * Mock out static files when running in Jest (elastic#13315) * Add services to moduleNameMapper in jest config.
…lastic#13479) * Rename tasks to createComponent and documentComponent. * Reference correct src paths in README. * Add children and className to templates' propTypes. * Add default folder name for page demo. * Add suffix to sandbox routes. * Specify testPathIgnorePatterns more clearly. * Rename component.test.js to test.js so that Jenkins won't try to run it. * Update npm scripts to depend on local yo dependency, not global. * Add ui_framework/src to copy task. * Simplify npm scripts and remove requirement for installing Yeoman from README. * Mock out static files when running in Jest (elastic#13315) * Add services to moduleNameMapper in jest config.
…13479) * Rename tasks to createComponent and documentComponent. * Reference correct src paths in README. * Add children and className to templates' propTypes. * Add default folder name for page demo. * Add suffix to sandbox routes. * Specify testPathIgnorePatterns more clearly. * Rename component.test.js to test.js so that Jenkins won't try to run it. * Update npm scripts to depend on local yo dependency, not global. * Add ui_framework/src to copy task. * Simplify npm scripts and remove requirement for installing Yeoman from README. * Mock out static files when running in Jest (#13315) * Add services to moduleNameMapper in jest config.
Static assets should like css-files should be mocked out, so Jest doesn't interpret them as javascript.
Related Jest documentation: https://facebook.github.io/jest/docs/webpack.html#handling-static-assets