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

[2.0] Fix loopback in PhantomJS, fix karma tests #344

Merged
merged 2 commits into from
Jun 26, 2014
Merged

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Jun 23, 2014

  • Move configuration of Karma unit-tests from Gruntfile.js to a
    standalone file (test/karma.conf.js).
  • Add a new Grunt task karma:unit-ci to run Karma unit-tests in
    PhantomJS and produce karma-xunit.xml file that can be consumed
    by the CI server.
  • Add grunt karma:unit-ci to the test script in package.json.
  • Add es5-shim module to karma unit-tests in order to provide
    ES5-methods required by LoopBack.
  • Fix mixin(source) in lib/loopback.js to work in PhantomJS.
    Object.getOwnPropertyDescriptor() provided by es5-shim does not
    work in the same way as in Node.

Requires loopbackio/loopback-datasource-juggler#152

/to @ritch Please review.
/cc @rmg When this patch is landed, loopback is ready to run karma unit-tests on our Jenkins. npm test is already running karma via Grunt, is there anything else needed to enable karma tests on Jenkins?

@rmg
Copy link
Member

rmg commented Jun 23, 2014

@bajtos you can test what Jenkins will do by using sl-ci-run locally.

What kind of report is generated by the karma tests?

@bajtos
Copy link
Member Author

bajtos commented Jun 23, 2014

@rmg I am always forgetting about sl-ci-run. Ok, the Karma tests were run. The output is karma-xunit.xml in the project root dir.

@@ -80,6 +80,10 @@ function createApplication() {
function mixin(source) {
for (var key in source) {
var desc = Object.getOwnPropertyDescriptor(source, key);

// Fix for legacy (pre-ES5) browsers like PhantomJS
if (!desc) continue;
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this breaking?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK it is not breaking. This check is preventing the following situation:

$ node
> Object.defineProperty({}, 'foo', undefined)
TypeError: Property description must be an object: undefined
    at Function.defineProperty (native)

desc is not defined when the key is not an own property of the source. MDN on getOwnPropertyDescriptor:

Return value
A property descriptor of the given property if it exists on the object, undefined otherwise.

What happens in PhantomJS:

  • for (var key in source) enumerates over bind function
  • the bind function is not an own property, getOwnPropertyDescriptor returns undefined
  • Object.defineProperty throws when the descriptor is not defined

@ritch
Copy link
Member

ritch commented Jun 23, 2014

@bajtos Are we including the es5 shim when not running the tests (eg. when built by browserify)?

@rmg
Copy link
Member

rmg commented Jun 23, 2014

karma-xunit.xml will be picked up by CI, but I have a feeling the xunit.xml file is no longer generated.

@rmg
Copy link
Member

rmg commented Jun 23, 2014

test please

@bajtos
Copy link
Member Author

bajtos commented Jun 24, 2014

@ritch Are we including the es5 shim when not running the tests (eg. when built by browserify)?

No. Most browsers support ES5 these days, with the exception of Internet Explorer 6-8.

yo angular is adding a conditional include to the generated index.html file:

    <!--[if lt IE 9]>
    <script src="bower_components/es5-shim/es5-shim.js"></script>
    <script src="bower_components/json3/lib/json3.min.js"></script>
    <![endif]-->

Even if this was not the case, including ES5 shim in the browserified bundle would not be a good idea, as it would unnecessarily increase the size of the browser bundle for all browsers.

I suppose this matter should be described in the documentation. The docs should mention that loopback depends on ES5 and users should load ES5-shim to run it in a browser that does not implement ES5.

/cc @crandmck

@bajtos
Copy link
Member Author

bajtos commented Jun 24, 2014

@rmg I have a feeling the xunit.xml file is no longer generated.

Yes, that's correct. I tried to change the test script to mocha && grunt karma:unit-ci, but it did not help. What do you advise?

@ritch
Copy link
Member

ritch commented Jun 24, 2014

Even if this was not the case, including ES5 shim in the browserified bundle would not be a good idea, as it would unnecessarily increase the size of the browser bundle for all browsers.

Sure.

I suppose this matter should be described in the documentation. The docs should mention that loopback depends on ES5 and users should load ES5-shim to run it in a browser that does not implement ES5.

We should have a guide for using loopback in browsers that includes this.

@bajtos bajtos added doc and removed doc labels Jun 24, 2014
@bajtos bajtos added this to the 2.0.0 milestone Jun 24, 2014
@rmg
Copy link
Member

rmg commented Jun 24, 2014

@bajtos the Gruntfile could have a top-level test task that runs tests based on the environment. It could check for $JENKINS_HOME and tell karma and mocha to generate xunit test reports (matching **/*xunit.xml) and produce a more developer friendly output when not under CI. I tried to avoid the need for that with the build framework our CI uses, but multiple test runners makes the automatic detection/replacement for overriding reporters more complicated.

@bajtos
Copy link
Member Author

bajtos commented Jun 24, 2014

@rmg that sounds plausible, I'll look into it tomorrow. thanks!

@bajtos
Copy link
Member Author

bajtos commented Jun 25, 2014

@rmg @ritch I have implemented grunt test task in the commit above, could you please review?

@rmg
Copy link
Member

rmg commented Jun 25, 2014

@bajtos it looks like I'll have to make changes to sl-ci-run so that it doesn't assume mocha and bypass npm test. If you could make mocha-and-karma the name of the task it should work without modifying sl-ci-run. In the future it might also be useful to use mocha-jenkins-reporter but that shouldn't be necessary right now.

@bajtos
Copy link
Member Author

bajtos commented Jun 26, 2014

@slnode test please

Miroslav Bajtoš added 2 commits June 26, 2014 13:30
- Move configuration of Karma unit-tests from `Gruntfile.js` to a
  standalone file (`test/karma.conf.js`).

- Add a new Grunt task `karma:unit-ci` to run Karma unit-tests in
  PhantomJS and produce karma-xunit.xml file that can be consumed
  by the CI server.

- Add grunt-mocha-test, configure it to run unit-tests.

- Add `grunt test` task that runs both karma and mocha tests,
  detects Jenkins to produce XML output on CI server.

- Modify the `test` script in `package.json` to run
  `grunt mocha-and-karma` (an alias for `grunt test`).
  The alias is required to trick `sl-ci-run` to run `npm test`
  instead of calling directly `mocha`.

- Add `es5-shim` module to karma unit-tests in order to provide
  ES5-methods required by LoopBack.

- Fix `mixin(source)` in lib/loopback.js to work in PhantomJS.
  `Object.getOwnPropertyDescriptor()` provided by `es5-shim` does not
  work in the same way as in Node.
@bajtos
Copy link
Member Author

bajtos commented Jun 26, 2014

grunt mocha-and-karma seems to work well.

bajtos added a commit that referenced this pull request Jun 26, 2014
[2.0] Fix loopback in PhantomJS, fix karma tests
@bajtos bajtos merged commit dd5360a into 2.0 Jun 26, 2014
@bajtos bajtos deleted the fix-phantomjs-tests branch June 26, 2014 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants