Unable to run istanbul-mocha tests when referencing clappr

[iongion]

Using clappr 0.2.52

The command I use to start the tests:

node_modules/.bin/babel-node ./node_modules/babel-istanbul/lib/cli cover node_modules/mocha/bin/_mocha -- --compilers js:babel-core/register --bail --sort --reporter spec --require test/bootstrap.js --recursive test/unit/**/**/*.spec.js

But they suddenly stop running with:

W:\Workspace\app\node_modules\clappr\src\main.js:5
import Player from 'components/player'
^^^^^^

SyntaxError: Unexpected token import
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:387:25)
    at Module._extensions..js (module.js:422:10)
    at Object.require.extensions.(anonymous function) [as .js] (W:\Workspace\app\node_modules\babel-register\lib\node.js:166:7)

Which looks as if babel is not being used.
My .babelrc has nothing special:

{
  "presets": [
    "es2015",
    "react"
  ],
  "env": {
    "development": {
      "presets": ["react-hmre"]
    }
  },
  "plugins": [
    [
      "babel-root-import",
      {
        "rootPathPrefix": "~",
        "rootPathSuffix": "sources/modules"
      }
    ],
    [
      "babel-plugin-transform-builtin-extend",
      {
        "globals": [
          "Error"
        ]
      }
    ]
  ]
}

Do you have any ideas why this happens ?

[iongion]

In fact, just running the tests with babel-node

node_modules/.bin/babel-node test/unit/modules/app/EntryPoint.spec.js

import Player from 'components/player'
^^^^^^

SyntaxError: Unexpected token import
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:387:25)

Where EntryPoint.spec.js is:

// node
// vendors
import chai from 'chai';
import Clappr from 'clappr';
// project
// locals
chai.should();
const expect = chai.expect;

describe('EntryPoint', () => {
  describe('#boot', () => {
    it('can boot', () => {
      const received = true;
      const expected = true;
      expect(expected).to.be.equal(received);
    });
  });
});

Showing that the loader is unable to import clappr as ES6 module, what can be done ?

[iongion]

Apparently, it happens due to the fact the the babel transpiler does not transpile anything in node_modules so clappr's main.js is not transpiled, that is why the import error.

In the end, the mocha command is altered a bit:

... --bail --sort --require test/bootstrap.js --reporter spec --recursive test/unit/**/**/**.spec.js

So that it bootstraps using a file where babel compiler is registered and its settings configured.

require('babel-core/register')({
  ignore: /node_modules\/(?!clappr)/,
});
require('babel-polyfill');

Now, the pain is still not over as the first problem I found was with babel-plugin-add-module-exports that cannot be found by babel.
I have added that to the list of my dependencies but then:

Error: Cannot find module 'components/player'

Because Clappr doesn't use file relative imports the babel compiler cannot find where components/player exist

Do you guys have any recommendations to get out of this mess ?

[leandromoreira]

@tjenkinson and @towerz are pretty skillful in these subject.

[tjenkinson]

@iongion I haven't looked at this in detail but I presume you're trying to run our tests during your build process?

I'm not sure why you need to do this. I think the general assumption is that dependencies that you pull from npm should already have been tested and work, and therefore if there are tests they should already all pass.

I.e I think you should test your own app code, but not worry about running any tests on any dependencies that may exist in the node_modules folder. You should assume the dependencies you have will work.

What do you think?

[iongion]

@tjenkinson - I see it is deeper than meets the eyes. Well, this is my workflow, I have a react/redux application and I am following best practices described here http://redux.js.org/docs/recipes/WritingTests.html#components

Now, I have a simple react component in which I wrap the Clappr player:

// node
// vendors
import React from 'react';
import ReactDOM from 'react-dom';
import Clappr from 'clappr';
// project

class MediaPlayer extends React.Component {
  componentDidMount() {
    this.playerNode = ReactDOM.findDOMNode(this);
    this.player = this.createPlayer(this.playerNode);
    this.refreshMediaSource();
  }
  shouldComponentUpdate(nextProps) {
    return (this.props.media === null) || (this.props.media.source !== nextProps.media.source);
  }
  componentDidUpdate() {
    this.refreshMediaSource();
  }
  componentWillUnmount() {
    if (this.player) {
      this.player.destroy();
    }
    if (this.playerNode) {
      const range = document.createRange();
      range.selectNodeContents(this.playerNode);
      range.deleteContents();
    }
  }
  refreshMediaSource() {
    if (this.props.media) {
      if (this.props.media.source) {
        this.loadVideo();
      } else {
        this.unloadVideo();
      }
    }
  }
  loadVideo() {
    if (this.props.media && this.props.media.source) {
      this.player.options.fps = this.props.media.fps;
      this.player.load(this.props.media.source);
      this.player.play();
    } else {
      // TODO: Unexpected request to play an empty media or no source
    }
  }
  unloadVideo() {
    this.player.stop();
  }
  createPlayer(parentNode) {
    const player = new Clappr.Player({
      parent: parentNode,
      plugins: {},
      autoPlay: false,
      hideMediaControl: false,
      preload: 'auto',
      disableKeyboardShortcuts: true,
    });
    return player;
  }
  render() {
    return (
      <div className="mediaPlayer"></div>
    );
  }
}

MediaPlayer.propTypes = {
  media: React.PropTypes.shape({
    source: React.PropTypes.string,
    width: React.PropTypes.integer,
    height: React.PropTypes.integer,
    fps: React.PropTypes.float,
  }),
};

export default MediaPlayer;

According to the Redux guide on how to test React components, I need to have something like this:

// node
// vendors
import chai from 'chai';
import React from 'react'
import TestUtils from 'react-addons-test-utils'
// project
import MediaPlayer from 'app/MediaPlayer';
// locals
chai.should();
const expect = chai.expect;

function setup() {
  let props = {
    media: expect.createSpy()
  }
  let renderer = TestUtils.createRenderer()
  renderer.render(<MediaPlayer {...props} />)
  let output = renderer.getRenderOutput()

  return {
    props,
    output,
    renderer
  }
}

describe('components', () => {
  describe('MediaPlayer', () => {
    it('should render correctly', () => {
      const { output } = setup()
      expect(output.type).toBe('div');
      expect(output.props.className).toBe('mediaPlayer');
    });
  });
});

Now, when I import MediaPlayer in my test, then of course it will also import Clappr and here the storm of problems start to happen.

On the webpack build path, I have no issue as Clappr is mentioned as external dependency and I load it from CDN with local fallback. For testing I have no clue how to specify a "reference" to the player, I use know patterns from previous expected environments such as java, .net, ruby, python .. I expect a import should suffice, but in our world, we see this is such a pain. What do you guys think ? What is your opinion on how this should be approached ?

[iongion]

I could mock Clappr and pass it as a dependency to the MediaPlayer as a static member, something like MediaPlayer.concreteImplementation = Clappr; - but that would be absolutely horrible :D as I would need to invent DI for my app and a bootstrapping sequence and all the java apartus, I do not need that stuff - I still hope it is an ES6 import/loader problem

[tjenkinson]

@iongion I understand it's because the source we push to npm is es6. I think really our source should be es5 (http://mammal.io/articles/using-es6-today/) because I don't think developers that install an npm package today should be expected to do something to it before it can be required into their own application.

@towerz can we not point main property in package.json to ./dist/clappr.js? Then developers won't have to worry about transforming clappr before they can use it.

I think one of the reasons for setting it to the source directory was so that developers could require individual source files into their projects. However I think specific components should be required from what is exported in "main.js". I.e require("clappr").mediaControl. Anything that's not exported from the entry point should be considered private.

This makes sense for where webpack is going with http://www.2ality.com/2015/12/webpack-tree-shaking.html

[iongion]

@tjenkinson That would be awesome! I have put a question on Babel's Slack channel and the guys there also mentioned this, that the published npm should be ES5.

What do you think about the discussion here ?

http://stackoverflow.com/questions/29738381/how-to-publish-a-module-written-in-es6-to-npm

They are discussing of keeping your code as is but in pre-publish you actually build and that is what other will import with their import Clappr from clappr, solving the issue.

[tjenkinson]

@iongion I think we should point to the built es5 Clappr.js file, instead of creating a lib folder with all the files converted individually.

This is because with the files converted it still isn't valid es5 as webpack does some magic to make it valid es5 when it builds, like tracing import paths from our fake root, and converting imports of asset files to imports of generated modules which export urls.

[iongion]

@tjenkinson As long as they are not the ES6 files, it is good 👍 - Perfect!

[tjenkinson]

@iongion I think they will still need to be valid or you'll have issues.

I would presume that

var playIcon = require('icons/01-play.svg');

for example would throw an error because it will probably look for "icons" in the wrong place, and I think it will fail trying to require an svg.

[iongion]

In case where ttf files or svg icons must loaded, couldn't they use a relative path ?

var playIcon = require('./icons/01-play.svg');

[tjenkinson]

@iongion yes they would need to but we use webpack which handles all this for us.

After webpack

import playIcon from 'icons/01-play.svg'

would essentially become this:

var playIcon = "assets/some-file.svg";

which is now valid es5.

If we just transformed the src directory es5 then we would get

var playIcon = require('icons/01-play.svg');

which is still invalid.

[iongion]

But what is playIcon - isn't it an internal asset of Clappr ? Should devs be able to import it ? It is only if you want other devs to import it.

The problems that I faced while swithcing main script to ./dist/main.js are window and navigator that are used as globals in the dist/clappr.js

I try to use this to see if I can get over the issues with something like this https://www.npmjs.com/package/mock-browser

[tjenkinson]

@iongion playIcon gets set to the url of the actual svg file during the webpack build process, as webpack converts the code to es5, but also handles assets, css, and other things like the paths from root.

You can see this in the webpack config files here and here.

From what I understand our source code isn't technically ES6 because we make use of the webpack loaders to tweak things during the build, so our code only ever becomes valid javascript after webpack has finished.

I think devs should only be able to load modules that are exported from the entry point, which is what they get when they require("clappr").

I think mocking the browser specific features seems like the right way to go as the library is intended to run in a browser, not as a nodejs app, and I guess this also applies for your project too?

[iongion]

@tjenkinson I understood you from the first place, it happens the same in my projects, the final version of valid JS is the one webpack outputs. And yes, you are right about the entry point.

A little reporting of what I have done:

1. Modified package.json of clappr to specify main script as ./dist/clappr.js
2. Changed the bootstrap.js file of the test runner to this:

require('babel-core/register')();
require('babel-polyfill');
// required by Clappr - it asumes the existence of window, window.navigator and navigator
const jsdom = require('jsdom');
const DEFAULT_HTML = '<html><script src="script.js"></script><body></body></html>';
global.document = jsdom.jsdom(DEFAULT_HTML);
global.window = document.defaultView;
global.navigator = window.navigator;
global.getComputedStyle = () => {};

And it is true, this is ugly - but the alternative is to mock Clappr completely or to have Clappr behave more friendly in a non-browser environment such as NodeJS

[tjenkinson]

@iongion I agree I think mocking out like you did there would work, but Clappr being able to run in node would be better so you don't need to mock the browser.

I've just had a look in the code and there aren't too many places where window is actually used, so it might be possible to make it still run if window is undefined.

Sidenote if #1008 gets merged I don't think <script src="script.js"></script> would be needed anymore in your mock html.

[iongion]

Awesome @tjenkinson - whenever you guys are ready, be careful, it is not only window as global, in clappr you also use navigator on its own and getComputedStyle .. at least these I've identified

[tjenkinson]

@iongion yes after I wrote it I was thinking and some of the project also relies on dom events so it might be a bit less feasible and mocking the browser might be the best way.

We could maybe provide a nice method though to set a new window instance?

E.g.

var Clappr = require("clappr");
Clappr.window = myFakeWindow;

where Clappr.window defaults to window if it exists. Then anywhere where window is accessed directly would be changed to retrieve the value that has been set there. Things like navigator would need to be changed to windowImpl.navigator etc.

[tjenkinson]

Untested

// mocks.js

var windowImpl = window || null;
var hasBeenAccessed = false;

var toExport = {};
Object.defineProperty(toExport, 'window', {
  get: function()  {
    hasBeenAccessed = true;
    if (!windowImpl) {
       throw new Error("Need an implementation of 'window' to run.");
    }
    return windowImpl;
  },
  set: function(newWindowImpl) {
    if (hasBeenAccessed) {
       throw new Error("Cannot set window implementation after it has been accessed in the project.");
    }
    windowImpl = newWindowImpl;
  },
  enumerable: true,
  configurable: false
});
module.exports = toExport;

// main.js
import mocks from 'mocks'
// everything else

export default {
   mocks,
   // everything else
}

// something in the app that needs the window
import mocks from 'mocks';

var window = mocks.window;

When you import Clappr to use you would do.

var Clappr = require("clappr");
Clappr.mocks.window = myFakeWindow;

[iongion]

Looks legit - global variables are Always bad :D

[towerz]

@tjenkinson I agree with you, the main entry should point to dist/clappr.js. This way whoever uses the clappr npm package would be able to do so without needing to resort to a transpiler.

However, I disagree that we should drop the baseUrl parameter. In my opinion, it is a cleaner way to dynamically determine the assets path without depending on webpack or using global variables. Since it does not interfere with how webpack works with urls, I think it's still a better solution than using a global asset base url variable, like the one implemented on #1008.

[tjenkinson]

@iongion I have been thinking of this again and actually think maybe creating a stub of Clappr in your tests would be the best way.

Like I said at the beginning you should be able to assume that Clappr will work as documented and that there won't be any incompatible changes to the api between minor versions, so stubbing it should be fine. If I had a project which used twitter for example I would make a stub of the twitter api so it wasn't relying on making network requests etc.

As for making the stub be loaded for the tests instead of the real implementation you can look at https://github.com/thlorenz/proxyquire . It has a few warnings for cases when it won't work, but it's worked fine for me several times. It means you don't have to change your source as that module replaces the require function to perform some trickery.

For integration tests I'd suggest then either running the tests in a real browser, like Clappr does, or mocking the browser environment in node like you did before.

This sound ok?

[iongion]

Yes, it sounds good, but you still must declare as main script a compiled version if clappr, not the es6

[tjenkinson]

Great. That change has already happened in the version that was released yesterday :)

On 3 Jun 2016, at 06:40, ionut stoica notifications@github.com wrote:

Yes, it sounds good, but you still must declare as main script a compiled version if clappr, not the es6

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[iongion]

Awesome, closing this, you guys are amazing and Github really needs a discussion facillity so much and not issues. Great work!