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

Return new assertion with flags copied over instead of this. Fix #791 #799

Merged

Conversation

vieiralucas
Copy link
Member

@vieiralucas vieiralucas commented Sep 14, 2016

Hello!

This PR addresses #791.

  • overwriteProperty
  • overwriteMethod
  • addChainableMethod
  • overwriteChainableMethod

@@ -4,7 +4,9 @@
* MIT Licensed
*/

var chai = require('../../chai');
Copy link
Member

Choose a reason for hiding this comment

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

Does this create a circular dependency? Have we done this before? I feel like this might create issues

Copy link
Member

Choose a reason for hiding this comment

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

Currently addMethod and addProperty have this too and there's been no issue reported about it. However, I'd like to have more time to check this before giving a confident response.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused how it's working.

  1. test/bootstrap/index.js requires index.js
  2. index.js requires lib/chai.js
  3. lib/chai.js requires lib/chai/utils/index.js
  4. lib/chai/utils/index.js requires lib/chai/utils/addProperty.js
  5. lib/chai/utils/addProperty.js requires lib/chai.js

Thus circular dependency. -scratches head-

Copy link
Member

Choose a reason for hiding this comment

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

I think it works because chai.js only sets module.exports and this file only calls stuff from chai.js inside of the methods, so it all kind of works out eventually.

@vieiralucas vieiralucas force-pushed the consistency-return-new-assertion branch from 68a4b4f to 21df19c Compare September 14, 2016 13:15
@vieiralucas vieiralucas force-pushed the consistency-return-new-assertion branch from 21df19c to 4101fdf Compare September 14, 2016 16:39
@keithamus keithamus added this to the 4.0 milestone Sep 14, 2016
vieiralucas added a commit that referenced this pull request Sep 15, 2016
Hello there,

While I was working on #799, I discovered that you cannot call `JSON.stringify` on an assertion because o proxify.

```javascript
const { expect } = require('chai');

const equal = expect(1).to.equal(1);
JSON.stringify(equal); // This will throw Error: Invalid Chai property: toJSON
``
@vieiralucas vieiralucas force-pushed the consistency-return-new-assertion branch from 1d05b40 to fe8f2ee Compare September 15, 2016 02:48
@vieiralucas
Copy link
Member Author

vieiralucas commented Sep 15, 2016

I believe that I was creating more confusion than explaining something (at least to myself).

IMHO, we should return a new Assertion with flags copied over not only to prevent bugs with length, but to keep consistency as well.

@vieiralucas vieiralucas force-pushed the consistency-return-new-assertion branch from fe8f2ee to c1d5a54 Compare September 15, 2016 04:01
@vieiralucas vieiralucas force-pushed the consistency-return-new-assertion branch from 36dae89 to 1ae0a41 Compare September 15, 2016 05:34
Copy link
Contributor

@meeber meeber left a comment

Choose a reason for hiding this comment

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

@vieiralucas Awesome work sir, thank you for doing this! I tested this branch with my work-in-progress on proxifying methods, and it solves the problem I was running into. Yay!

I noticed you added delete statements to a bunch of existing tests that were adding new properties. I agree that we should make sure every test cleans up after itself. My only suggestion is that we restructure some of these tests so that they clean up after themselves even if they fail. Therefore, we'll need to wrap those tests in describe blocks and move the cleanup code into after blocks, as I noticed some of the newer tests already do.

@@ -230,6 +230,8 @@ describe('utilities', function () {
});

expect(expect('foo').result()).to.equal('result');

delete chai.Assertion.prototype.result;
Copy link
Contributor

@meeber meeber Sep 19, 2016

Choose a reason for hiding this comment

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

It's a great idea to add this so the test cleans up after itself, but we need it to run the cleanup operation regardless of success or failure. Therefore, I think we should wrap this test in a describe with an after function to contain the cleanup activities.

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course, you're absolutely right. Thanks for noticing.
I'll make the changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -273,6 +275,9 @@ describe('utilities', function () {

var anotherAssertion = expect([1, 2, 3]).to.have.a.lengthOf(3).and.to.be.ok;
expect(anotherAssertion.length.constructor).to.equal(assertionConstructor);

delete chai.Assertion.prototype.returnNewAssertion;
delete chai.Assertion.prototype.checkFlags;
Copy link
Contributor

@meeber meeber Sep 19, 2016

Choose a reason for hiding this comment

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

Same thing here; cleanup steps should be in an after function.

@@ -403,6 +444,8 @@ describe('utilities', function () {

var assert = expect('chai').to.be.tea;
expect(assert.__flags.tea).to.equal('chai');

delete chai.Assertion.prototype.tea;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here; cleanup steps should be in an after function.

@@ -446,6 +489,9 @@ describe('utilities', function () {

expect(expect([1, 2, 3]).be).to.be.an.instanceOf(assertionConstructor);
expect(expect([1, 2, 3]).thing).to.be.an.instanceOf(assertionConstructor);

delete chai.Assertion.prototype.thing;
delete chai.Assertion.prototype.checkFlags;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here; cleanup steps should be in an after function.

@@ -456,11 +502,16 @@ describe('utilities', function () {
});

expect(expect('foo').result).to.equal('result');

delete chai.Assertion.prototype.result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here; cleanup steps should be in an after function.

@@ -477,6 +528,8 @@ describe('utilities', function () {
expect(matcha.__flags.tea).to.equal('matcha');
var assert = expect('something').to.be.tea;
expect(assert.__flags.tea).to.equal('chai');

delete chai.Assertion.prototype.tea;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here; cleanup steps should be in an after function.

@@ -489,6 +542,8 @@ describe('utilities', function () {
});

expect(expect('foo').result).to.equal('result');

delete chai.Assertion.prototype.result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here; cleanup steps should be in an after function.

@@ -762,46 +866,156 @@ describe('utilities', function () {
expect(obj).x.to.be.ok;
expect(obj).to.have.property('__x', 'X!');
})

delete chai.Assertion.prototype.x;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here; cleanup steps should be in an after function.

expect(expect('bar').foo('bar')).to.be.an.instanceOf(assertionConstructor);

delete chai.Assertion.prototype.foo;
delete chai.Assertion.prototype.checkFlags;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here; cleanup steps should be in an after function.

@vieiralucas vieiralucas changed the title [WIP] Return new assertion with flags copied over instead of this. Fix #791 Return new assertion with flags copied over instead of this. Fix #791 Sep 19, 2016
@meeber
Copy link
Contributor

meeber commented Sep 19, 2016

@vieiralucas Thanks for the extra effort refactoring the tests to clean up after themselves. Personally, I think the whole test suite could use a refactoring to be better organized and labeled, and only have one assertion per test, but that's a future battle :D

Anyway, this LGTM! @keithamus @lucasfcosta?

Edit: I think the LGTM feature is a little wonky since github added the "Review" system. I haven't done much reading on the Review system. Can it be configured to require multiple approved reviews instead of just one? If so, it could probably replace LGTM altogether.

@lucasfcosta
Copy link
Member

@meeber I totally agree with your considerations about the LGTM feature, it would be perfect if we could require more than one review for PR, however, if this is not possible, can we disable this and continue using only LGTM?

Also, this LGTM!

@lucasfcosta lucasfcosta merged commit 7683356 into chaijs:master Sep 19, 2016
@keithamus
Copy link
Member

@meeber @lucasfcosta I enabled the github review to see how well it worked. I've disabled it for now, and we can keep using LGTM. When it's configurable for N number of reviewers then we'll look at re-enabling I think 😄

@vieiralucas vieiralucas deleted the consistency-return-new-assertion branch September 19, 2016 14:34
@meeber
Copy link
Contributor

meeber commented Sep 19, 2016

Based on my limited interactions with it, I think the builtin review tool has a lot of potential. I particularly like being able to group a bunch of comments together before submitting them during a review.

@keithamus
Copy link
Member

Yes, I think ultimately it's better than LGTM (for our use case) but also much less configurable - namely the number of required reviewers, which becomes a deal breaker (unless we want to lower the number of required viewers to 1 😆).

@vieiralucas
Copy link
Member Author

vieiralucas commented Sep 19, 2016

Personally I did not find a way to tell if the changes requested were already made.
I miss the "commented on an outdated diff".
I also think that once the changes are approved, the request should be hidden. IMHO currently the new review tool makes the PR discussion to polluted.

Maybe I'm just a hater 😸

@lucasfcosta
Copy link
Member

lucasfcosta commented Sep 19, 2016

@vieiralucas @meeber @keithamus Apparently this makes the build fail, I have no idea how this is possible since the Travis check passed.
I'll try to run tests for this and fix things as soon as I get home.

Any ideas about how this happened? Shouldn't the Travis check be the same as the builds?

@keithamus
Copy link
Member

Ahh sadly they're not - because the saucelabs tests only run on master (because if they were to run on pull requests, we'd risk exposing our saucelabs tokens to malicious PR code).

@vieiralucas
Copy link
Member Author

Not very familiarized with saucelabs, but I found this

IE 10.0.0 (Windows 7 0.0.0) utilities overwriteChainableMethod should return a new assertion with flags copied over FAILED

@keithamus
Copy link
Member

You should try to run the tests in IE10 locally, see why it is failing.

@meeber
Copy link
Contributor

meeber commented Sep 19, 2016

I can't look at it closely right now but my quick guess is it's because IE 10.0 takes a different code path here, causing this test to fail.

Edit: If that guess is correct, then a work around might be to only run that test if __proto__ support is detected.

@vieiralucas
Copy link
Member Author

Currently downloading IE10 VM.
26 min left...

@vieiralucas
Copy link
Member Author

Well...
Now I know how to run tests on IE. 😸

WHY IS THAT SO HARD MICROSOFT PLS

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.

4 participants