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

Throwing an error on missing selector in test harness trigger #320

Merged
merged 3 commits into from Apr 23, 2019

Conversation

rorticus
Copy link
Contributor

@rorticus rorticus commented Apr 22, 2019

Type: bug / feature

The following has been addressed in the PR:

Description:

Instead of silently failing, throwing an error when a h.trigger encounters a selector that doesn't return any nodes.

Resolves #318

Copy link
Contributor

@aciccarello aciccarello left a comment

Looks great! Probably should be considered a breaking change.

}

if (!firstItem) {
throw new Error(`Cannot find node with selector ${selector}`);
Copy link
Contributor

@aciccarello aciccarello Apr 22, 2019

Choose a reason for hiding this comment

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

I like this error message. It might be good to make this and the assertion template errors consistent.

throw Error('Node not found');

Copy link
Contributor Author

@rorticus rorticus Apr 22, 2019

Choose a reason for hiding this comment

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

I don't disagree, but updating the message there is a bit more complicated. I've made the change though, we'll see how it goes 😄

if (!node) {
throw Error('Node not found');
throw Error(`Cannot find node with selector ${selector}`);
}
if (!isWNode(node) && !isVNode(node)) {
throw Error('Cannot set or get on unknown node');

Choose a reason for hiding this comment

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

Hmm I don’t think we should change this. The findOne is meant to be equivalent to querySelector and not throw. It also doesn’t make sense to throw a setting unknown node here given the function name?

Copy link
Contributor Author

@rorticus rorticus Apr 22, 2019

Choose a reason for hiding this comment

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

Hehe, yeah, I wasn't sure about these changes. The other option is i can pass the selector in guard so it can use it in the error message, but thats going to end up looking silly and be a bit of a pain every time you call it. Maybe i should just remove that last commit?

Choose a reason for hiding this comment

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

Ha yeah I agree, maybe for now we just stick with the trigger change and we can raise a separate issue for making error messages more consistent (it might be that we use the same function in the harness and assertion template)

@rorticus rorticus merged commit 6897fa2 into dojo:master Apr 23, 2019
4 checks passed
@rorticus rorticus deleted the issue-318 branch Apr 23, 2019
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 this pull request may close these issues.

3 participants