Testing

[nucleartide]

Hey dude! First of all, this addon is great - I'm really liking the pretty default styles out of the box.

One question I have though: in acceptance tests, Ember waits for the run loop to clear before continuing. I notice that when I write an acceptance test for notifications, I have to wait clearDuration amount of time before my test continues. This slows down the test and is a little annoying.

It seems like these two methods are causing that to happen. So my resolution was to allow those methods to run, only if Ember isn't in test mode. (Using the Ember.testing flag would also work, instead of a having to manually toggle a variable on the service.)

Do you think it's a good idea to incorporate this change into ember-cli-notifications? Alternatively, I can simply extend the notifications service (as part of this recent change), and make the method overrides myself.

I'd love to hear what you think!

[ynnoj]

@nucleartide Hey man! Thanks for the interest in the add-on and your kind words 🙃

One area of the add-on that I am particularly keen to improve on is testing (see #91). I have little experience in writing tests, so I'd be more than happy to accept contributions that will improve this aspect of the add-on.

If you're interested in helping me make the add-on bulletproof, then fire away with the PRs 🚀

[nucleartide]

Ah okay! I will fire away with the PRs should I run into problems. :)

[luketheobscure]

Was any progress made on this? Ran into the same issue. My current workaround is probably going to be to set setDefaultAutoClear to false during acceptance tests, and manually clearing out the notices as I go.

[nucleartide]

my solution was to extend the notifications service, override methods involving the run loop, and remove calls to the run loop. but this was a long time ago.

hopefully that helps!

[ctcpip]

This issue should get resolved in this addon, but in the meantime, here is my workaround: We check the environment variable to see if we are testing, and if so, set the auto clear duration to 0.

// controllers/application.js

import Ember from 'ember';
import ENV from 'myApp/config/environment';

export default Ember.Controller.extend({
  notifications: Ember.inject.service('notification-messages'),
  init: function () {

    const notifications = this.get('notifications');

    notifications.setDefaultAutoClear(true);

    if(ENV.environment === 'test') {
      notifications.setDefaultClearDuration(0);
    }

  }
});

[sbatson5]

That's a good workaround for clearing the images, but makes it hard if you want to actually test the notifications in an acceptance test. I had a defaults hash that I overwrite in test: messageOptions: ENV.environment == 'test' ? {} : DEFAULT_OPTIONS,

Then wrote a helper that dismisses the notifications. That way the notifications stick around during the life of your test and you can test that the right messaging appears (if that's important to you).

[concertiv]

Even with setDefaultClearDuration at 0, the close animation slows down acceptance tests. I guess this is because removeNotification has a hard-coded 500ms delay? https://github.com/stonecircle/ember-cli-notifications/blob/master/addon/services/notification-messages-service.js#L72

[jelhan]

Did you tried out to set clearDuration to 1ms in testing environment? I'm using this approach and didn't faced any issues with it so far.

// config/environment.js
module.exports = function(environment) {
  let ENV = {
    'ember-cli-notifications': {
      autoClear: true
    }
  }

  if (environment === 'test') {
    ENV['ember-cli-notifications'].clearDuration = 1;
  }

  return ENV;
};

[concertiv]

We're doing something similar with the same result, but our issue isn't clearDuration.

The delay is hardcoded at 500ms in the run.later here: https://github.com/stonecircle/ember-cli-notifications/blob/ac80976e648814dc2b1804d0e3b169e1e9463083/addon/services/notification-messages-service.js#L80-L82

[jelhan]

I see. Didn't noticed that one.

In my opinion this dismissal animation duration should also be configurable. Maybe call it a dismissalAnimationDuration or to be more general delayRemovingOnClose. In this case it would not only be possible to set this one to 1ms for testing but also to have a different duration if needed for custom styling. @mansona, @ynnoj: Would you accept a PR for this one?

I'm also not quite sure why the default is 500ms. The animation is only 250ms isn't it?

https://github.com/stonecircle/ember-cli-notifications/blob/ac80976e648814dc2b1804d0e3b169e1e9463083/addon/styles/components/notification-message.css#L30

[mistahenry]

Another work around, in your tests/test-helper, reopen the class and patch the remove

import NotificationMessageService from 'ember-cli-notifications/services/notification-messages-service';
NotificationMessageService.reopen({
    removeNotification(notification) {
        if (!notification) {
          return;
        }
    
        notification.set('dismiss', true);
        this.removeObject(notification);
    }
})