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

fix: .include to work with all objects #1012

Merged
merged 1 commit into from
Aug 2, 2017

Conversation

meeber
Copy link
Contributor

@meeber meeber commented Jul 30, 2017

Overly strict type-checking was causing .include to reject Error
objects and objects with a custom @@toStringTag.

Fixes #1009

@meeber meeber requested a review from a team as a code owner July 30, 2017 13:06
vieiralucas
vieiralucas previously approved these changes Jul 31, 2017
Copy link
Member

@vieiralucas vieiralucas left a comment

Choose a reason for hiding this comment

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

Awesome as always

LGTM

return;
// `_.expectTypes` isn't used here because `.include` should work with
// objects with a custom `@@toStringTag`.
if ((objType !== 'string' && objType !== 'object') || objType === 'null') {
Copy link
Contributor

@shvaikalesh shvaikalesh Jul 31, 2017

Choose a reason for hiding this comment

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

Maybe we should move this check to default to eliminate !== 'string' check and use val === Object(val) to avoid === 'null' and allow functions?

Overly strict type-checking was causing `.include` to reject `Error`
objects and objects with a custom `@@toStringTag`.
@meeber
Copy link
Contributor Author

meeber commented Jul 31, 2017

@shvaikalesh @vieiralucas Updated.

@shvaikalesh
Copy link
Contributor

Great job, @meeber. LGTM.

@keithamus keithamus merged commit b625497 into chaijs:master Aug 2, 2017
@meeber meeber mentioned this pull request Aug 4, 2017
@meeber meeber deleted the fix-include-types branch August 6, 2017 13:47
@metareven
Copy link

@meeber

This does not work. The following object still gives the error message:

AssertionError: object tested must be an array, a map, an object, a set, a string, or a weakset, but object given

When using:
expect(object).to.include('notReady')

{ id: 674,
  title: 'El Globo',
  floors: [ 4 ],
  topFloors: 0,
  ui: 
   { statusBarMenuVisible: false,
     statusBarMenuInitial: true,
     loggedInUsersMenuVisible: false,
     loggedInUsersMenuInitial: true,
     lockLabelOn: false },
  status: 
   { isPublishable: true,
     isBeingPublished: false,
     isDirty: false,
     published: false,
     stored: false },
  itemId: 1,
  createdBy: 'kurator2',
  createdDate: 1476885553000,
  latest: true,
  live: false,
  parentId: undefined,
  notReady: 
   { isHouseLockedByAnother: true,
     isAlreadyPublished: false,
     isAlreadyStored: false,
     isSingleFloor: false,
     isTemporaryFloor: false,
     isSingleRoom: false },
  type: undefined 
}

Tested on both chai 4.1.1 and chai 4.1.2

@meeber
Copy link
Contributor Author

meeber commented May 10, 2018

@metareven According to the docs, I don't think you can use .include in this way. Since your target is an object, the value you pass has to be an object too, not a string. This should work instead for your usage: expect(object).to.have.property('notReady');.

With that said, something is clearly not right with this error message and this PR in general. It's been quite awhile since I looked at the Chai codebase, but I'm thinking if (val !== Object(val)) { should be if (obj !== Object(obj)) { instead, and then a separate check is needed afterward to make sure val is an object as well with a more precise error message if it's not.

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.

5 participants