Merge rebased 4.x.x branch into master #783

meeber · 2016-09-05T22:17:00Z

I ran into one minor issue while rebasing the 4.x.x branch:

There was a test in the 4.x.x branch that relied on being able to access an Assertion object's flags after the assertion had failed and been caught in a try/catch block. However, a change in the master branch made it so that assertions return a new Assertion object with flags transferred over, instead of returning the original Assertion object. This means that any subsequent flags are set on the new Assertion object instead of the original. For a successful assertion, this is fine, since the new Assertion object is returned. But in this case, for a failed assertion that throws an error and is caught in a try/catch block, the test no longer has access to any of the failed Assertion object's flags that were added after the first step in the assertion.

I resolved this issue in an extra commit that fixes this broken test by getting rid of the .to chain so that the test has access to the relevant Assertion object and its flags.

In addition to the issue above, I made a couple of side notes:

A few tests broke in Node v6.5 due to a change in how anonymous functions that are assigned to a variable appear in failed assertion error messages. This issue is completely unrelated to 4.x.x. I'll be following up this PR with a separate one that addresses that.
There's now a large divergence between the behavior of the .property and .ownProperty assertions due to heavy refactoring of .property in master and minor refactoring of .ownProperty in the 4.x.x branch. I'll be following up this PR with a separate one that addresses that.

(Closes #772)

keithamus · 2016-09-06T09:37:43Z

This looks like some truly great work @meeber. LGTM, tagging @lucasfcosta for the merge.

Also interesting to see how much of 4.0 has been driven by @lucasfcosta - looks like many commits in that PR. Congrats on the stellar work, both of you!

meeber · 2016-09-06T10:44:00Z

@keithamus Thank you sir, and agreed regarding Professor Lucas's contributions. Most of them were made before I joined the team, so this was my first time reviewing them. Unsurprisingly, they were excellent commits across the board! And everything was well-documented, so I was able to understand and resolve all conflicts during the rebase. Thank you @lucasfcosta!

lucasfcosta · 2016-09-06T12:11:07Z

Yay! I'm happy to hear that! Thank you so much guys!
Actually, you are the ones who are awesome, I could not have made those commits without your help. Your ideas and reviews were essential to guarantee the quality of that work.
I said that yesterday on twitter, but I'd like to repeat it: I learned a lot by reading your code and comments.
I'll use my time this weekend to look at this with attention and make sure everything is okay.

Thanks for the work Mr. @meeber and Mr. @keithamus! It's an honor to be a part of this great team!

Fixes: chaijs#498

lucasfcosta · 2016-09-10T00:38:29Z

lib/chai/core/assertions.js

@@ -679,14 +684,15 @@ module.exports = function (chai, _) {
  /**
   * ### .least(value)
   *
-   * Asserts that the target is greater than or equal to `value`.
+   * Asserts that the number target is greater than or equal to `value`.


I know this is a merge, but I'll just leave this here as a note:
Shouldn't it be written Asserts that the **target number** ... instead of Asserts that the **number target** ...?

Yeah, the reason it was phrased that way is because that's how it was already phrased elsewhere in the code (see: #692 (comment)). I think the thing for us to do is submit a separate PR that rephrases all of them (both old and new) at once.

Ahhh, I remember that now!
Thanks for refreshing my memory!
I totally agree with you, that would be a great idea, I will open an issue for that just so we won't forget.

lucasfcosta · 2016-09-10T01:27:15Z

lib/chai/core/assertions.js

   *
   *     expect(10).to.be.at.least(10);
   *
   * Can also be used in conjunction with `length` to
   * assert a minimum length. The benefit being a
   * more informative error message than if the length
-   * was supplied directly.
+   * was supplied directly. In this case the target must
+   * have a length property.
   *
   *     expect('foo').to.have.length.of.at.least(2);
   *     expect([ 1, 2, 3 ]).to.have.length.of.at.least(3);


Another side note here:

Currently we can't do:
expect([ 1, 2, 3 ]).to.have.a.length.of.at.least(3);
Because the a word returns a function which has a property called .length and there is no way we can remove that in some environments, but we could use a Proxy in environments that do support it to override what happens when trying to access length on the function returned by addChainableBehavior, what do you think? Is this possible? I didn't dig deep on this one, but I think it might be a valid strategy.

Yup, because function.length is configurable in modern environments, it can be intercepted with proxies or manually deleted without the use of proxies. I think one of them is worth doing (in a separate PR) but I'm not sure which approach is best.

lucasfcosta · 2016-09-10T17:48:32Z

I just went through it all and it LGTM!
Excellent work @meeber (as you always do)! 😄
I left a couple of minor non-merge-related notes of things that should be done after this PR gets merged.

meeber · 2016-09-10T18:36:28Z

@lucasfcosta Thanks for the careful review! I think this is ready to merge. Do you want to do the honors? :D

lucasfcosta · 2016-09-10T18:49:00Z

@meeber Yes! Of course, I haven't seen @keithamus already approved this, thanks for noticing!

HERE WE GO!

meeber · 2016-09-10T19:03:24Z

@lucasfcosta Superb use of Ren and Stimpy gif.

lucasfcosta · 2016-09-10T19:07:10Z

Thanks @meeber, it took years of practice 😄

meeber mentioned this pull request Sep 5, 2016

Fix broken tests in Node v6.5 and update supported node versions #784

Merged

keithamus modified the milestone: 4.0 Sep 6, 2016

Dai and others added 19 commits September 8, 2016 09:37

Added handling for undefined and null (fails on negate flag too)

e0ee3de

Updated code based on discussion

128c112

handle NaN properly in assertions

d2884a6

Fixes: chaijs#498

Deep members comparisons show diff.

a1190c9

No longer silently allowing undefined method calls. Fixes chaijs#467.

04aa795

New showDiff behaviour also depending on expected and actual values

0a88696

isTypedArray verification

f20c7b5

formatTypedArray function

8123bf7

Truncate long typedArray

b90ba4e

Minor inspect code style fixes

aae52f2

Fix truncateThreshold

4d314b6

Use regex.test method to check for TypedArrays

3ff3cc7

Use getOwnPropertyDescriptor instead of __lookup methods

0e68e8d

Truncate typedArrays according to their string representation

0118949

Assertion subject (obj) changes when using ownProperty assertion

16101af

ownProperty assertions on Assert interface

df1a892

Verify types on within and related assertions

c3e8285

Documentation update to indicate type requirements

62309d3

Fix broken test after rebasing 4.x.x branch

4a009de

meeber force-pushed the 4.x.x branch from 95f1165 to 4a009de Compare September 8, 2016 13:40

lucasfcosta reviewed Sep 10, 2016
View reviewed changes

lucasfcosta mentioned this pull request Sep 10, 2016

Rephrase old docs and use target <type> instead of <type> target #792

Closed

lucasfcosta reviewed Sep 10, 2016
View reviewed changes

lucasfcosta merged commit 8166f33 into chaijs:master Sep 10, 2016

meeber deleted the 4.x.x branch August 6, 2017 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge rebased 4.x.x branch into master #783

Merge rebased 4.x.x branch into master #783

meeber commented Sep 5, 2016 •

edited

Loading

keithamus commented Sep 6, 2016

meeber commented Sep 6, 2016

lucasfcosta commented Sep 6, 2016

lucasfcosta Sep 10, 2016 •

edited

Loading

meeber Sep 10, 2016

lucasfcosta Sep 10, 2016

lucasfcosta Sep 10, 2016

meeber Sep 10, 2016

lucasfcosta commented Sep 10, 2016

meeber commented Sep 10, 2016

lucasfcosta commented Sep 10, 2016

meeber commented Sep 10, 2016

lucasfcosta commented Sep 10, 2016

Merge rebased 4.x.x branch into master #783

Merge rebased 4.x.x branch into master #783

Conversation

meeber commented Sep 5, 2016 • edited Loading

keithamus commented Sep 6, 2016

meeber commented Sep 6, 2016

lucasfcosta commented Sep 6, 2016

lucasfcosta Sep 10, 2016 • edited Loading

Choose a reason for hiding this comment

meeber Sep 10, 2016

Choose a reason for hiding this comment

lucasfcosta Sep 10, 2016

Choose a reason for hiding this comment

lucasfcosta Sep 10, 2016

Choose a reason for hiding this comment

meeber Sep 10, 2016

Choose a reason for hiding this comment

lucasfcosta commented Sep 10, 2016

meeber commented Sep 10, 2016

lucasfcosta commented Sep 10, 2016

meeber commented Sep 10, 2016

lucasfcosta commented Sep 10, 2016

meeber commented Sep 5, 2016 •

edited

Loading

lucasfcosta Sep 10, 2016 •

edited

Loading