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

Make the <function> type a subtype of <object> #448

Merged
merged 35 commits into from Dec 26, 2018

Conversation

Projects
None yet
3 participants
@papandreou
Copy link
Member

papandreou commented Apr 16, 2018

Looks like this will work if/when we fix and land #328

FIXME:

  • <function> to (throw|error) <function>

@papandreou papandreou added the WIP label Apr 16, 2018

@alexjeffburke alexjeffburke referenced this pull request Jul 1, 2018

Closed

Unexpected 11 #491

5 of 5 tasks complete

@papandreou papandreou force-pushed the feature/functionSubtypeOfObject branch from 02fc5df to ca9c8b7 Dec 22, 2018

@papandreou papandreou changed the base branch from master to v11 Dec 22, 2018

papandreou added some commits Dec 22, 2018

Fix most of the failing tests by increasing the specificity of the <a…
…ny|Error> to [exhaustively] satisfy <function> assertion, which probably shouldn't even be there
Wrap function in expect.it to avoid relying on the ...to satisfy <fun…
…ction> assertions in one case

1 failing:
  1) to satisfy assertion
       should render a diff when the function differs:
function => expect.it
5 failing:
  1) to satisfy assertion
       forwards normal errors to the top-level:
  2) to satisfy assertion
       should render a diff when the function differs:
  3) to satisfy assertion
       should not break when the assertion fails and the subject has a property that also exists on Object.prototype:
  4) to satisfy assertion
       with the not flag
         should fail when a non-Unexpected error occurs:
  5) to satisfy assertion
       on Error instances
         when satisfying against a function
           fails when the function throws:
Fix one test that should have used expect.it
4 failing:
  1) to satisfy assertion
       forwards normal errors to the top-level:
  2) to satisfy assertion
       should render a diff when the function differs:
  3) to satisfy assertion
       should not break when the assertion fails and the subject has a property that also exists on Object.prototype:
  4) to satisfy assertion
       on Error instances
         when satisfying against a function
           fails when the function throws:
Fix another test that should have used expect.it
3 failing:
  1) to satisfy assertion
       should render a diff when the function differs:
  2) to satisfy assertion
       should not break when the assertion fails and the subject has a property that also exists on Object.prototype:
  3) to satisfy assertion
       on Error instances
         when satisfying against a function
           fails when the function throws:
Make sure that <object> to satisfy <function> fails, fix test
7 failing:
  1) to satisfy assertion
       should render a diff when the function differs:
  2) to satisfy assertion
       should not break when the assertion fails and the subject has a property that also exists on Object.prototype:
  3) to satisfy assertion (NEW!)
       on Error instances
         when satisfying against a function
           should succeed if the function does not throw:
  4) to satisfy assertion
       on Error instances
         when satisfying against a function
           fails when the function throws:
  5) to satisfy assertion (NEW!)
       on Buffer instances
         should satisfy a function:
  6) to throw assertion (NEW!)
       given a function the function is called with the exception:
  7) when passed as parameters to assertion (NEW!)
       with the constructor flag
         should create a new instance:

@papandreou papandreou removed the WIP label Dec 24, 2018

@papandreou

This comment has been minimized.

Copy link
Member

papandreou commented Dec 24, 2018

I got this to work now, but it involved:

  • Fixing a bunch of tests that still relied on function in the RHS being executed, which should have been sorted out in #328. In some cases they were still being called, but the change to the type hierarchy surfaced it.
  • Removing some ... to satisfy <function> assertions that should have been removed in #328.
  • Only extract "own" keys from the RHS object in <object> to [exhaustively] satisfy <object>. Previously we would also extract Object.prototype properties and match against them iff they were present in the subject. In one test this bug was masked by the in some places still present semantics of functions in the RHS, and since {foo: 123}.constructor(123) doesn't throw, the assertion didn't fail.

Along the same lines it removes the <any|Error> to [exhaustively] satisfy <function> assertion.

I can try to pick these apart if it's mixing too many things together.

Also, we could consider alternatives to the addition of the own parameter to hasKey.

... And some more tests would be nice, I guess.

@papandreou

This comment has been minimized.

Copy link
Member

papandreou commented Dec 25, 2018

I tried and failed to split this up into chunks, but added some tests now :)

@papandreou papandreou force-pushed the feature/functionSubtypeOfObject branch from da89eab to 58be731 Dec 25, 2018

@papandreou

This comment has been minimized.

Copy link
Member

papandreou commented Dec 25, 2018

The coverage drop in assertions.js and types.js is due to covered lines being removed, no new uncovered lines are being introduced.

@alexjeffburke
Copy link
Member

alexjeffburke left a comment

This is looking seriously good - have added one comment to the "to satisfy" change.

Seeing all the test suite changes together make it very clear that this change together with the previous expect.it() stuff make things far more consistent. Specifically banning the problematic "to satisfy " case is excellent, and since we spoke of a conversion guide we could easily spit out a URL to a page that describes this stuff if need be.

return key in obj;
hasKey(obj, key, own) {
if (own) {
return Object.prototype.hasOwnProperty.call(obj, key);

This comment has been minimized.

@alexjeffburke

alexjeffburke Dec 26, 2018

Member

Just to be clear, I guess the use of hasOwnProperty from the Object is to avoid having any problems with objects that might have been created using Object.create(null) such that their prototype chain is empty?

I'm trying to find points of reference for this new own situation - I guess it's a consequence of getting literally all the possible property names. I've got this niggling feeling tat this conditional should be contained entirely within "to satisfy" rather than end up in hasKey, but I cannot yet figure out why yet..

This comment has been minimized.

@papandreou

papandreou Dec 26, 2018

Member

Just to be clear, I guess the use of hasOwnProperty from the Object is to avoid having any problems with objects that might have been created using Object.create(null) such that their prototype chain is empty?

Yeah, correct 👍

This comment has been minimized.

@papandreou

papandreou Dec 26, 2018

Member

I've got this niggling feeling tat this conditional should be contained entirely within "to satisfy" rather than end up in hasKey

Yeah, I feel the same way. All the valueType stuff that has to do with satisfying against a subtype of object is a bit weird, maybe we don't actually want it.

papandreou added some commits Dec 26, 2018

Merge pull request #542 from unexpectedjs/feature/unsupportToHaveItem…
…sWithFunction

Feature/unsupport to have items with function
Merge branch 'v11' into feature/functionSubtypeOfObject
* v11:
  Paralellize the Travis build, by splitting the main target
  Increase the Karma timeout to 1 minute
  Stop testing jest on all Node versions
  Updated the changelog
  10.40.0
  Build unexpected.js
  Specify Node version numbers as strings instead of floats
  Added BrowserStack badges
  Fix IE tests and remove special casing for PhantomJS
  Don't run the IE11 browser tests in the Chrome headless build
  No need to build with 8.4.0 twice
  Update build matrix, run browser tests separately
  Update rollup to version 0.68.1
  Initial browser stack setup
  Use the progress reporter for karma
  Added karma, mocha, chrome headless setup
  Update rollup-plugin-node-resolve to version 4.0.0
  Fix special casing of UnexpectedError in <function> to error/throw
@sunesimonsen
Copy link
Member

sunesimonsen left a comment

👍 💯 greatness.

@papandreou papandreou merged commit a10e8d7 into v11 Dec 26, 2018

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.001%) to 98.005%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@papandreou papandreou deleted the feature/functionSubtypeOfObject branch Dec 26, 2018

@sunesimonsen sunesimonsen referenced this pull request Jan 3, 2019

Merged

V11 (Major) #509

25 of 25 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment