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

Add tests for SSR to prevent regressions #370

Closed
justingreenberg opened this issue Dec 14, 2016 · 6 comments
Closed

Add tests for SSR to prevent regressions #370

justingreenberg opened this issue Dec 14, 2016 · 6 comments

Comments

@justingreenberg
Copy link

Since the plan is to ship SSR/isomorphic rendering support with v1.4.0, there should probably be some minimal functional tests to prevent regressions as users begin to depend on this feature.

Reference: #360, wiki cc/ @adidahiya @codebling (nice work dude)

@codebling
Copy link
Contributor

codebling commented Dec 15, 2016

Agreed. Thanks for opening this, I was worried we'd lose track of it.

So biggest issue here is that we can't use Karma to run this test (because it runs tests in the browser), the way we do for the rest of the project. So we're definitely stuck making a new Gulp task to run the SSR tests. I think we can get away with just using the default Mocha runner & reporter.

The test itself is fairly trivial. We simply
ReactDOMServer.renderToString(React.createElement(Component, props));
If that succeeds, The component should be safe for server-side rendering and isomorphic use.

The more complicated aspect is how to add the tests. What do you think is the correct approach here? We could:

  • Manually create the same single test for every component
    • Risk of server-side test being forgotten for new components
      • Could add separate coverage to mitigate this,
        • but this would probably involve also finding a way to mark server-side test coverage as ignored, in case we need to leave components out
    • We can easily leave out tests for components that should not be tested (if need be)
    • We'll need to figure out where this test should be created: in the same test file or in a separate test file.
  • Automatically run the same test for every component using a new Gulp task
    • Ensures that the test will not be left out for new components
    • We'll need to figure out a way to leave components out, if need be (manually adding exclusions to the task sounds terrible)
    • We'll need to find a way to programmatically locate component code
  • Automatically generate the same test for every new component, using a new Gulp task or as part of the build task.
    • Has the same considerations as automatically running the tests for every component
  • Something else?

@justingreenberg
Copy link
Author

justingreenberg commented Dec 15, 2016

Automatically run the same test for every component using a new Gulp task

This probably makes the most sense for now... /examples might be a good target for automated ssr tests since they provide implementations for all framework components as @codebling indicated, test itself can be pretty straightforward, eg render to string in node

// pseudocode - please excuse brevity/typos
// test/SSRTest.tsx
const loadExamples = directory =>
  fs.readdirSync(directory)
    .filter(fileName => fileName.match(/(.*Example).tsx$/ig))
    .map(filePath => require(`${path.resolve(dir, filePath)}`))

export default SSRTestComponent extends CustomBaseClass {
  protected renderExample() {
    return(<div>{loadExamples(EXAMPLES_DIR).map(Example => <Example />)}</div>)
  }
}

//...outside karma

ReactDOMServer.renderToString(<SSRTestComponent />)

@codebling
Copy link
Contributor

codebling commented Dec 17, 2016

@justingreenberg I don't like using the examples as test targets, since they're (1) a manual build artifact that has no coverage measurements, and (2) an afterthought (may only be added in the build after a new component is introduced). I do like that all the components are readily available there.

After thinking about this for a while, I realised that Blueprint exposes all components by hierarchically importing all of its children and reexporting them in a single file (for example, in packages/core/index.ts). While this is unusual, because most packages make you import individual files from the package (case in point: const ReactDOMServer = require("react-dom/server");), it works to our advantage.

We would also like to avoid cascading failures. For example, if we were working on the <Popover> and broke server rendering for that class, <Tooltip> would also fail. The <Tooltip> test case failure is a false positive and can make testing annoying. Thankfully, It just so happens that the React Test Utilities' shallow renderer has exactly what we need. It renders on the server, running through the standard server-side rendering life cycle, but doesn't render children.

So I rigged up a little something that just imports the root index, gets all of the exported keys, tests to see if they are React Components and if they are, tests them for server compatibility by rendering them using the shallow renderer. Output looks like this:

[19:47:58] Using gulpfile blueprint/Gulpfile.js
[19:47:58] Starting 'isotest'...
[19:47:58] Starting 'typescript-compile-core'...
[19:47:59] core: compiling 64 typescript files
[19:48:03] Finished 'typescript-compile-core' after 4.69 s
[19:48:03] Starting 'isotest-core'...


  Alert
    v can be server-rendered

  Button
    v can be server-rendered

  AnchorButton
    v can be server-rendered

  Collapse
    v can be server-rendered

  CollapsibleList
    v can be server-rendered

  Dialog
    v can be server-rendered

  EditableText
    v can be server-rendered

  Checkbox
    v can be server-rendered

  Switch
    v can be server-rendered

  Radio
    v can be server-rendered

  InputGroup
    v can be server-rendered

  RadioGroup
    v can be server-rendered

  Hotkey
    1) can be server-rendered

  KeyCombo
    2) can be server-rendered

  Hotkeys
    3) can be server-rendered

  Menu
    v can be server-rendered

  MenuDivider
    v can be server-rendered

  MenuItem
    v can be server-rendered

  NonIdealState
    v can be server-rendered

  Overlay
    v can be server-rendered

  Popover
    4) can be server-rendered

  SVGPopover
    v can be server-rendered

  Portal
    v can be server-rendered

  ProgressBar
    v can be server-rendered

  SVGTooltip
    v can be server-rendered

  RangeSlider
    v can be server-rendered

  Slider
    v can be server-rendered

  Spinner
    v can be server-rendered

  SVGSpinner
    v can be server-rendered

  Tab
    v can be server-rendered

  Tabs
    v can be server-rendered

  TabList
    v can be server-rendered

  TabPanel
    v can be server-rendered

  Tag
    v can be server-rendered

  Toast
    v can be server-rendered

  Toaster
    v can be server-rendered

  Tooltip
    v can be server-rendered

  Tree
    v can be server-rendered

  TreeNode
    v can be server-rendered


  35 passing (61ms)
  4 failing

There are 4 failures right now, Components for which we'll need to create specific test cases (and exclude them from the autotest). Working on that.

@adidahiya
Copy link
Contributor

@codebling that looks like a pretty sane approach as long as errors thrown in JS cause the test suite to fail (I believe this is the case now).

@codebling
Copy link
Contributor

codebling commented Dec 18, 2016

@adidahiya it is (they do). I have a PR more or less ready (you can have a look at my fork).

@codebling
Copy link
Contributor

I'll just make sure that it works on Circle, then I'll open the PR.

I'm also looking at catching React render warnings (like Warning: Each child in an array or iterator should have a unique "key" prop.) and forcing those to fail the tests, which currently doesn't happen. This can easily be a separate PR, though.

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

No branches or pull requests

3 participants