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

jshint error for 'describe/it not defined' on generated helper tests #37

Closed
DisruptiveMind opened this issue Feb 9, 2015 · 25 comments · Fixed by #42
Closed

jshint error for 'describe/it not defined' on generated helper tests #37

DisruptiveMind opened this issue Feb 9, 2015 · 25 comments · Fixed by #42

Comments

@DisruptiveMind
Copy link

When generating a new helper the associated test does not run due to globals 'describe' and 'it' not being defined.

Generated

/* jshint expr:true */
import {
  aHelper
} from 'hub/helpers/a-helper';

describe('AHelperHelper', function() {
  // Replace this with your real tests.
  it('works', function() {
    var result = aHelper(42);
    expect(result).to.be.ok;
  });
});

Results in:

unit/helpers/a-helper-test.js: line 6, col 1, 'describe' is not defined.
unit/helpers/a-helper-test.js: line 8, col 3, 'it' is not defined.

solution....sort of
I manually added 'describe' and 'it' to _tests/.jshintrc_ after looking at #3

Is there another way of doing this or am I hooking things up wrong somewhere else?

@jonathanKingston
Copy link
Member

👍 Would appreciate the correct path to import 'describe' from. The generators here don't specify one.

@DisruptiveMind
Copy link
Author

I just added describe & it inside _tests/.jshintrc_ and it seems to have solved it for now.
I can generate helper helper-name without the generated tests tripping JSHint now.

tests/.jshintrc

{
  
  "predef": [
    
    "describe",
    "it"
    
  ],
  
}

@jonathanKingston
Copy link
Member

Ember-cli 0.1.13 update is trying to move away from any global like this: http://reefpoints.dockyard.com/2015/02/06/ember-qunit-0-2.html

https://github.com/ember-cli/ember-cli/releases/tag/v0.1.13

@rwjblue
Copy link
Member

rwjblue commented Feb 13, 2015

I think this should be handled up stream, and created an issue in ember-mocha for it: emberjs/ember-mocha#22

@jonathanKingston
Copy link
Member

@rwjblue good call, thanks for raising.

Is it worth raising about exposing assert, should and expects from chai.js too?

@rwjblue
Copy link
Member

rwjblue commented Feb 13, 2015

Is it worth raising about exposing assert, should and expects from chai.js too?

Not sure, I'm not a heavy ember-mocha user, I was mostly trying to help point @dgeb and others towards a possible solution (that we pulled into ember-qunit).

@dgeb
Copy link
Member

dgeb commented Feb 16, 2015

Thanks @rwjblue

@jonathanKingston @DisruptiveMind et al - PRs welcome over in ember-mocha as spec'd out in emberjs/ember-mocha#22

Is it worth raising about exposing assert, should and expects from chai.js too?

Seems like an additional top-level chai module would be most appropriate.

@jonathanKingston
Copy link
Member

@dgeb I'll make a PR for the describe at some point soonish unless someone else gets to it first.

I made a quick chai package to expose the main interfaces to chai. This might be a better fragmentation from just having ember-cli module define the use of Chai too: https://github.com/jonathanKingston/ember-cli-chai

@rwjblue
Copy link
Member

rwjblue commented Feb 22, 2015

Submitted emberjs/ember-mocha#23 to add the shims, this repo will still need to be updated (mostly blueprints I think) once that lands.

@DisruptiveMind
Copy link
Author

I've just added the import statements for the helpers blueprint in DisruptiveMind/ember-cli-mocha@afe5711. I'm holding off the PR for now.

Just wondering, should expect be imported as well?

@jonathanKingston
Copy link
Member

@DisruptiveMind I was thinking about this the other day, would it make more sense to have ember-cli-mocha more configurable so that any assertion library can be used. The addon could take a config argument for example, which would then modify the helpers accordingly.

So something like: config/environment.js:

ENV['ember-cli-mocha'] = {
  assertion: 'chai.assert' // Valid values: 'chai.assert', 'chai.should', 'chai.expects', 'qunit'
}

Would generate:

/* jshint expr:true */
 import {
  describe,
  it
} from 'ember-mocha';

import {
  assert
} from 'chai';

This might be something that can be made generic enough to be usable from ember-cli-qunit too.

What do you think @rwjblue.

@DisruptiveMind
Copy link
Author

@jonathanKingston 👍 on configurable options

@dgeb
Copy link
Member

dgeb commented Feb 22, 2015

Please upgrade to ember-cli-mocha 0.5.0 and then run ember g ember-cli-mocha to get the latest ember-mocha.

ember-mocha now exports shimmed mocha and chai modules.

Note that only expect and assert are exported from chai for now. should requires injections which most developers will want to avoid (perhaps we can figure out how to better support should at some point).

I've also updated the blueprints to use these new shims. I like the idea in theory of making these blueprints more flexible as you've described above, as long as we can do so without too much noise in the blueprints. If you want to pursue an approach, please open a new issue / PR.

@jonathanKingston
Copy link
Member

@dgeb 👍

Does doing the following with 'should' not work as expected:

  import should from 'ember-mocha';
  should();

  thing.should...

As far as I was aware exporting the should method then using this within a module scope should have a self contained impact.

@rwjblue
Copy link
Member

rwjblue commented Feb 22, 2015

@jonathanKingston - I am not an expert on chai but it seems that should actually adds a bunch of methods to the core object prototype which means using it anywhere would affect all the rest of the tests.

@rwjblue
Copy link
Member

rwjblue commented Feb 22, 2015

@rwjblue
Copy link
Member

rwjblue commented Feb 22, 2015

But I do agree that the chai shim should export should, and the actual assertion lib/mechanism is completely independent (as it should be).

@jonathanKingston
Copy link
Member

@rwjblue yeah it does, I guess I just assumed ES6 modules completely isolated their code even for native prototypes (It hasn't been something I had tried before because of it's edge usecase - however would certainly help if that was the expected behaviour and Babel just needed to implement).

@rwjblue
Copy link
Member

rwjblue commented Feb 22, 2015

@jonathanKingston - I'm pretty sure that ES6 does not suggest that modifications to core prototypes are isolated within each module (and if it did, I highly doubt that it could be polyfilled via babel).

@dgeb
Copy link
Member

dgeb commented Feb 22, 2015

Yeah, once should is invoked, its core object injections could affect other tests - even those that don't use should. I guess I see should as a footgun (i.e. anti-pattern), especially when presented alongside expect and assert as equivalents. I really am on the fence about exposing it from the chai shim - but I'd be willing to do so if there's sufficient demand.

@rwjblue
Copy link
Member

rwjblue commented Feb 22, 2015

I guess I see should as a footgun (i.e. anti-pattern), especially when presented alongside expect and assert as equivalents.

This is exactly why I left it out of my initial chai shim PR. I think if you really really want to use should syntax you can figure it out on you own. I would personally rather steer folks towards the right way...

@rwjblue
Copy link
Member

rwjblue commented Feb 22, 2015

And, I still haven't heard anyone say they wanted to use should syntax: unless I am mis-reading @jonathanKingston's comment he was mostly advocating for it to be pluggable (which it is already).

@dgeb
Copy link
Member

dgeb commented Feb 22, 2015

And, I still haven't heard anyone say they wanted to use should syntax

@rwjblue oh, I almost forgot about #14. Let me comment there.

@jonathanKingston
Copy link
Member

@rwjblue yes I am advocating for it to be pluggable (I was just trying to ascertain if it would be isolated).


From my quick scrambling through the ES6 spec (I'm probably not reading this right).

Object Environment Records

Immutable bindings do not exist for object environment records.

So no help there, however I believe that is just describing normal behaviour of scripts.


Well known intrinsic objects

Unless otherwise specified each intrinsic object actually corresponds to a set of similar objects, one per Realm.

Code Realms

Before it is evaluated, all ECMAScript code must be associated with a Realm. Conceptually, a realm consists of a set of intrinsic objects, an ECMAScript global environment

Module evaluation

Set the Realm of moduleCxt to module.[[Realm]].


My reading of that is Object is considered an Intrinsic object which is related to the Realm it is in, Modules appear to run in their own Realm. Creating a new Realm appears to reinstantiate new Intrinsics.

I could be wrong in how that is supposed to be read, however if that is the case Babel could be updated with this behaviour.

@dgeb I was just about to quote #14 👍

@jonathanKingston
Copy link
Member

Also:
Crossing realms with symbols

The problem is that each realm has its own local copy of Array and, because objects have individual identities, those local copies are considered different

So I will double check this issue exists in Babel, if it does then hopefully that can be fixed there to isolate the prototypes.

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

Successfully merging a pull request may close this issue.

4 participants