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

feat: extend error message details #112

Merged
merged 8 commits into from
Mar 17, 2020
Merged

Conversation

dotpointer
Copy link
Contributor

@dotpointer dotpointer commented Mar 13, 2020

Extend error message details so they tell more about what the error is about.

  • Tell the idPath that is being used, a common cause of error is a misconfigured idPath (id-path).

  • Tell the list data, a cause of error is an empty list or one that does not have the expected data.

  • Reformat the id and item data presentation so it explains what these are.

No issue.

@dotpointer dotpointer requested a review from a team March 13, 2020 17:11
@dotpointer dotpointer self-assigned this Mar 13, 2020
cosmoz-data-nav.js Outdated Show resolved Hide resolved
@dotpointer dotpointer requested review from cristinecula and a team March 14, 2020 13:11
@megheaiulian
Copy link
Collaborator

Tests are failling ...

@dotpointer dotpointer force-pushed the extend-error-messages-1 branch from 0db21eb to 6b4a7be Compare March 14, 2020 17:52
@dotpointer
Copy link
Contributor Author

Tests are failling ...

Tried replacing calledWithExactly with calledWithMatch in the tests since the log messages now contains objects. But the tests fail anyway.

How do I get the warn outputs that the spies compares with and how do I use calledWithMatch - I thought it wanted partial regular expressions?

@cristinecula
Copy link
Collaborator

@dotpointer calledWithMatch works with a sinon matcher: https://sinonjs.org/releases/latest/matchers/

@dotpointer
Copy link
Contributor Author

dotpointer commented Mar 15, 2020

@dotpointer calledWithMatch works with a sinon matcher: https://sinonjs.org/releases/latest/matchers/

Something like this?
assert.isTrue(warnSpy.calledWithMatch(sinon.match.regexp('/regular expression/')), true);

Pushed a change to try it but it says sinon.match.regexp is not a function even if it imports /components/sinon/pkg/sinon.js?

@cristinecula
Copy link
Collaborator

@dotpointer the documentations says: sinon.match(regexp);, so it should be:
calledWithMatch(sinon.match(/List item...

@megheaiulian
Copy link
Collaborator

Try assert.isTrue(warnSpy.calledWithMatch(sinon.match(/regular\sexpression/iu)), true);

@dotpointer
Copy link
Contributor Author

Try assert.isTrue(warnSpy.calledWithMatch(sinon.match(/regular\sexpression/iu)), true);

Pushed a fix with it now.

@dotpointer dotpointer force-pushed the extend-error-messages-1 branch from 5e8c036 to a4267ed Compare March 15, 2020 13:33
@dotpointer
Copy link
Contributor Author

@cristinecula @megheaiulian Still failing, also tried this to match anything without progress:

const warnSpy = sinon.spy(console, 'warn'),
    data = { id: 23 };
nav.setItemById('23', data);
assert.isTrue(warnSpy.calledWithMatch(sinon.match('/.*/iu')), true);
warnSpy.restore();

It finds false and also outputs Attempted to wrap warn which is already wrapped.

@cristinecula
Copy link
Collaborator

@dotpointer '/.*/iu' is not a regex, it's a string. Remove the quotes.

@dotpointer
Copy link
Contributor Author

dotpointer commented Mar 15, 2020

@dotpointer '/.*/iu' is not a regex, it's a string. Remove the quotes.

Tried assert.isTrue(warnSpy.calledWithMatch(sinon.match(/.*/iu)), true);, but it fails the same. Updated the tests.

@@ -506,11 +506,11 @@ class CosmozDataNav extends translatable(mixinBehaviors([IronResizableBehavior],

if (matches.length === 0) {
// eslint-disable-next-line no-console
console.warn('trying to replace an item that is not in the list', id, item);
console.warn('List item replacement failed, no matching idPath', this.idPath, 'with id', id, 'in the item list', items, 'to replace with item', item);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was wrong because console.warn() was called with multiple arguments ...

Copy link
Contributor Author

@dotpointer dotpointer Mar 17, 2020

Choose a reason for hiding this comment

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

This was wrong because console.warn() was called with multiple arguments ...

Not sure that I understand here, console.warn() can take multiple arguments like console.log() do?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dotpointer warn was called with multiple arguments, but the tests were asserting that it was called with a single argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, thanks.

@cristinecula cristinecula merged commit 602ac7c into master Mar 17, 2020
@cristinecula cristinecula deleted the extend-error-messages-1 branch March 17, 2020 08:43
github-actions bot pushed a commit that referenced this pull request Mar 17, 2020
# [3.2.0](v3.1.0...v3.2.0) (2020-03-17)

### Features

* extend error message details ([#112](#112)) ([602ac7c](602ac7c))
@github-actions
Copy link

🎉 This PR is included in version 3.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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