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

remove requirement for providing a message #260

Merged
merged 3 commits into from
Feb 19, 2018
Merged

remove requirement for providing a message #260

merged 3 commits into from
Feb 19, 2018

Conversation

st-h
Copy link
Contributor

@st-h st-h commented Feb 4, 2018

fixes #259

I figured it would be reasonable to keep the message property required when preventDuplicates is enabled as that feature relies on the message string.

Please let me know or feel free to modify the added test, if you know of any better way to assert a thrown error than that:

assert.throws((() => {
  service.add({ });
}).message, 'Assertion Failed: The flash message cannot be empty when preventDuplicates is enabled.');

Copy link
Collaborator

@sbatson5 sbatson5 left a comment

Choose a reason for hiding this comment

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

This looks great. Just one comment on the test and we should be good!

set(service, 'defaultPreventDuplicates', true);

assert.throws((() => {
service.add({ });
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think assert.throws is the right solution to test this. However, you seem to be getting a false positive. If you change this line to service.add({ message: 'foo' }) your test will still pass. I think this is due to the wrapped function.

I think you will want something like this:

  assert.throws(() => {
    service.add({ });
  },
  ({ message }) => {
    return message == 'Assertion Failed: The flash message cannot be empty when preventDuplicates is enabled.';
  },
    'Error is thrown'
  );

assert.throws can take 3 arguments. If you pass it only 2, the second is simply the assertion description, so it was not matching the error text here.

The second argument should be a function that accepts the error object and returns true or false. Give the above example a try and you should see it passes with no message and fails with one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, thanks for catching that. Should all be fixed now :)

customProperty: 'ohai'
});

assert.equal(get(service, 'queue.0.customProperty'), 'ohai');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor nitpick, but rather than queue.0 could we do queue.firstObject? I feel it reads just a bit more clear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, done :)

@sbatson5
Copy link
Collaborator

Awesome! :shipit:

@sbatson5 sbatson5 merged commit 7532006 into adopted-ember-addons:master Feb 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove requirement for providing a message
3 participants