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.

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

Fix CSS tests #839

Merged
merged 7 commits into from Jul 10, 2018
Merged

Fix CSS tests #839

merged 7 commits into from Jul 10, 2018

Conversation

yangshun
Copy link
Contributor

@yangshun yangshun commented Jul 8, 2018

Motivation

Our CSS integration tests have been broken long ago but because a fulfilled promise would not fail the test, we never caught that the test was failing. This PR:

  • Changes the way we write an async test
  • Test suite properly fail when one of the tests fail
  • Fix the broken CSS test

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

yarn test

Related PRs

NA

@yangshun yangshun requested a review from endiliey July 8, 2018 18:52
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jul 8, 2018
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jul 8, 2018

Deploy preview for docusaurus-preview ready!

Built with commit 5c030f3

https://deploy-preview-839--docusaurus-preview.netlify.com

@endiliey
Copy link
Contributor

endiliey commented Jul 8, 2018

I've suspected that its failing, I think this fixes unhandled css test promise #810

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

LGTM,

One concern that i have is that we have an extra file for an option. There is also something similar, exporting LIVE_RELOAD_PORT in core/constants.js

Maybe we can refactor later on as config file or maybe even refactoring into utils so the option is only defined in one file

Example:

// server/utils.js
async function minifyCss(cssContent) {
  const {css} = await cssnano.process(cssContent, {preset: 'default', zindex: false});
  return css;
}

module.exports = {
  minifyCss
};

Then we can do

const {minifyCss} = requires(./utils.js);
// ....
const css = await minifyCss(cssContent);

The good thing of this approach is that we can also add small utility test on the minifyCss function itself & its easy for us to replace cssnano with something else

Wdyt?

@@ -29,6 +30,7 @@ function getLanguage(file, refDir) {
const match = regexSubFolder.exec(file);

// Avoid misinterpreting subdirectory as language
const env = require('./env.js');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea here 😂But I think we'll need to change this back to top level when we move to ES6 as ES6 imports have to be top level.

@yangshun yangshun merged commit 4267337 into facebook:master Jul 10, 2018
@yangshun
Copy link
Contributor Author

Thanks so much for helping me fix the tests @endiliey! 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants