breakpoints aren't available in unit tests

[mminkoff]

I need to include the media service in my application controller unit test. However, as a unit test it doesn't get an application and so the breakpoints module isn't available (which took me about a day to realize). That's my bad. The solution was to also include 'breakpoints:main' in needs.

However, the error message is, as you might guess from the day-long diagnosis, not so helpful. Here's the problematic part:

// from tests/helpers/responsive.js
  _breakpointArr: Ember.computed('breakpoints', function() {
    return Object.keys(this.get('breakpoints')) || Ember.A([]);
  }),

I'm not entirely sure what the intention is or I'd submit a pull request.

First, I'd throw a more meaningful error if isBlank(this.get('breakpoints')), perhaps wondering if it's being included in a unit test and suggesting my fix above.

Second, I wonder what circumstances would allow the second half of the || to be reached. Even if it is, Object.keys wouldn't have returned an Ember.A, would it? So why the Ember.A here?

Thanks for taking a look.

[k-fish]

That's a good catch. It looks to just be a null guard, but you're right that breakpoints should never be null so long as the lookup succeeds.

Most of the testing assumes rendering is happening (acceptance / integration) as responsiveness usually falls entirely under presentation, in which you'd already have the app instance and injection happening for you.

I feel like an update to the README reminding users to add needs for unit tests would be sufficient, thoughts?

[mminkoff]

Thanks. Wouldn't it be better for the null guard to be this:

return Object.keys(this.get('breakpoints') || Ember.A([]));

instead of this?:

return Object.keys(this.get('breakpoints')) || Ember.A([]);

though I'm not sure what value Ember.A() adds over just plain {}.

A README update would certainly help but why not assert that breakpoints exists, with a helpful message? Even breakpoints is undefined would be more helpful than Cannot convert undefined or null to object, though I'd still rather breakpoints is undefined. Perhaps you're running a unit test and didn't add 'breakpoints:main' to your needs?. Given that there's a default breakpoints.js in the add-on, how likely is it that it's a different issue?

TBH just having this issue here should go a long way toward helping the next person who encounters the problem. :-)

[k-fish]

You're right, I didn't notice the guard was in the wrong place, that looks unintended 😄 .

TBH just having this issue here should go a long way toward helping the next person who encounters the problem. :-)

Totally agree, but your suggestion of an error message in this case would be even better. (eg. breakpoint lookup failed. If you are using this helper inside a unit test, you must add 'breakpoints:main' as a dependency (needs: ['breakpoints:main'...])). That and a readme entry should smooth this out for future users.

Would you happen to want to put up a PR? 😏

[mminkoff]

I would be happy to put up a PR! 😀 Shall I include the message as you wrote it above and move that pesky ) in the return? You want it to stay Ember.A([])? If breakpoints exists then Object.keys will get a POJO - why not have {} as the or?

[k-fish]

Awesome 🎉

Yeah you can use that message as-is. Get rid of the array, it would have been impossible to reach due to the error and how Object.keys works.