test(assert): increase coverage #1085

sukrosono · 2017-11-09T10:38:31Z

related issue #1084

codecov · 2017-11-09T10:40:29Z

Codecov Report

Merging #1085 into master will increase coverage by 0.73%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1085      +/-   ##
==========================================
+ Coverage   93.61%   94.34%   +0.73%     
==========================================
  Files          32       32              
  Lines        1628     1628              
  Branches      393      393              
==========================================
+ Hits         1524     1536      +12     
+ Misses        104       92      -12

Impacted Files	Coverage Δ
lib/chai/interface/assert.js	`93.04% <0%> (+3.2%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f54f71c...6eb1a50. Read the comment docs.

sukrosono · 2017-11-21T13:44:43Z

Since the msg is empty on changesBy, changesButNotBy, increasesBy, decreasesBy, doesNotDecreaseBy. I want to refactor them to not use tmpMsg. Please let me know if it's a wrong turn. I will only rely on travis test when refactor them.

Seems doesNotIncrease do it correctly. Since the author of #621 is @lucasfcosta, i like to listen to his opinion as well.

sukrosono · 2017-11-21T15:32:03Z

lib/chai/interface/assert.js

-   * Can be used to assert the inclusion of a subset of properties in an 
+   *
+   * Asserts that 'haystack' includes 'needle'.
+   * Can be used to assert the inclusion of a subset of properties in an


trailing space are removed by atom i guest. Would this affect documentation? If yes i will carefully add space on every next line. So next line will be *<space><space>...

Well, I would not worry about this that much even though we've already got #1042.

Alright, the overall explanation make it clearer. I will process when i had chance 😄 , glad to see you back. 💌

keithamus · 2017-11-22T11:49:25Z

test/assert.js

    assert.changesButNotBy(fnDec, obj, 'value', 1);
+    // assert.changesButNotBy(fnDec, getterFn, 'value', 1);


Why are these assertions commented out? Do they not work?

Yes Cap, it doesn't work. That's why i made #1090 , 😄

lucasfcosta

Hi @brutalcrozt,

First of all, I'd like to thank you for this PR! ❤️

However, I don't think these changes are compatible with our intentions with the changes/increases/decreases API.

There has been a misunderstanding.

As I've commented in my review, when providing a prop and a function we will end up checking for that prop in the function object. This is how the API is supposed to work.

If we've already got a getterFn then we don't need to specify any keys and it would not allow us to check for properties in functions.

lucasfcosta · 2017-11-22T22:16:26Z

lib/chai/interface/assert.js

      msg = tmpMsg;
+      prop = null;


I think this is not correct.

When we have four arguments and the type of obj is a function, we cannot set prop to null because the user might desire to check for the value of a prop in a function, since a function in JavaScript can have properties.

Please correct me if I'm wrong.

I will try to exclude this commit, focusing on testing only. Either we both wrong or none of us is wrong.

lucasfcosta · 2017-11-22T22:20:09Z

lib/chai/interface/assert.js

-      delta = prop;
-      msg = tmpMsg;
+      msg = 'getter function';
+      prop= null;


Same here, if we have four arguments to this I think that prop must not be null.

Also, a space is missing between prop and the = sign.

OMG, he notice 😄 , indeed that's my mistake. Did we need eslint?

lucasfcosta · 2017-11-22T22:22:08Z

lib/chai/interface/assert.js

-   * Can be used to assert the inclusion of a subset of properties in an 
+   *
+   * Asserts that 'haystack' includes 'needle'.
+   * Can be used to assert the inclusion of a subset of properties in an


Well, I would not worry about this that much even though we've already got #1042.

lucasfcosta · 2017-11-22T22:24:12Z

test/assert.js

        bangFn = function() { obj.str += '!' },
        smFn   = function() { 'foo' + 'bar' },
        batFn  = function() { heroes.push('batman') },
        lenFn  = function() { return heroes.length };

    assert.changes(fn, obj, 'value');
+    assert.changes(fn, getterFn, 'value');


I also think that this is incorrect.

When writing assert.changes(fn, getterFn, 'value') I understand that you expect fn to change the property value of getterFn.

It's redundant to specify a key if you already have a getter function. If we did this it would not be possible to check for property in function objects as well.

I think the misunderstanding is located here.

I found it awkward when we use assert(fn, getterFn, null, 1);

Which should be assert.changesBy(fn, getterFn, deltaValue);

I will continue discussing on issue 1090, kindly re-validate with your point of view.

Hi @brutalcrozt, thanks for the answer.

I'm sorry but I still didn't understand how this could be correct, since delta cannot be a string, which is what you're passing by using 'value' as the last argument.

Can anyone else please review & confirm just for us to make sure we're not missing any detail?

Hey brother, i think no need to ask third person point of view. I assume there is misunderstanding, i can say it correct just because run node test resulting green and travis also green. Have i show you this ?
Sorry i also make mistake again here, i mislead you. you are reviewing the assert.changes but i am comment about assert.changesBy.

I think that assert.changes pass because the key or property is be null when reaching this line.
do i need make additional change about this assert.changes ?
If you don't mind, please addressing assert.changesBy on other line. When you feel still necessary 😄 .

Ah, I understand this now. You were right about the tests passing.

The reason why I was confused was because of this line, which uses the third argument as a message when the second one is a function and we only have three arguments.

However, would you mind just changing the value string to example message or something like that? Just to make sure the test looks clear.

Great work, btw! Thanks 😄

PS.: IMO we should change this API in the future, otherwise we will make it mandatory for users to supply a message when they want to assert on a property present in a function, which is not that uncommon.

What do you think?

Done, let me know if this still require some change.

Agreed, we should add it to next milestone. 4.2.0 ?
Kinda related to #1090, but unfortunately i still have issue to formulate the word. Let's continue there 😉

lucasfcosta

Hi @brutalcrozt, thanks for the new commits, but I've seen that you just added a revert commit to this PR, but doing so basically resets all the feature changes right? Right now we've only got test changes.

I've seen the diff and the tests look a bit confusing btw. I made specific comments on them.

Let's make sure all the code is correctly indented too.

When reverting changes I highly recommend you to use rebase instead of adding commits. Rebase allows you to make the git log look cleaner and better without adding new unnecessary commits.

This video is a great way to learn about rebase btw.

If you need any help, please let me know.

lucasfcosta · 2017-11-23T17:09:13Z

test/assert.js

@@ -2251,26 +2251,33 @@ describe('assert', function () {
        heroes = ['spiderman', 'superman'],
        fn     = function() { obj.value += 5 },
        fnDec  = function() { obj.value -= 20 },
+        getterFn = function() { return obj.value },
+        getter= function() {},


Missing a space between = and the identifier.

lucasfcosta · 2017-11-23T17:09:22Z

test/assert.js

        bangFn = function() { obj.str += '!' },
        smFn   = function() { 'foo' + 'bar' },
        batFn  = function() { heroes.push('batman') },
        lenFn  = function() { return heroes.length };

+    getter.value= obj.value;


Missing a space here too. But this line will probably end up being deleted due to never using getter.

lucasfcosta · 2017-11-23T17:10:43Z

test/assert.js

@@ -2251,26 +2251,33 @@ describe('assert', function () {
        heroes = ['spiderman', 'superman'],
        fn     = function() { obj.value += 5 },
        fnDec  = function() { obj.value -= 20 },
+        getterFn = function() { return obj.value },
+        getter= function() {},


It also seems like this function is never used.

lucasfcosta · 2017-11-23T17:12:09Z

test/assert.js

    assert.changes(fn, obj, 'value');
+    assert.changes(fn, getterFn, 'value');


This does not seem correct.

If we want to test properties on functions we must create an fn that changes the value of a prop in the getterFn. What you've done here is create another getter function which has a value prop that is never used.

lucasfcosta · 2017-11-23T17:12:41Z

test/assert.js

    assert.changesBy(fn, obj, 'value', 5);
    assert.changesBy(fn, obj, 'value', -5);
+    assert.changesBy(fn, getterFn, 5);


Please take a look at the comment I've made on the first assertion test because that is valid to all additions below, such as this one.

so basically resets all the feature changes right?

Yes, sir.

Let's make sure all the code is correctly indented too.

Agreed.

Sorry that i have made unnecessary notification and work for you. I already take a look at your slack status about 5-10 min, because i already had bad experience with git rebase. So i ask Keith instead, but still i get lost on my own confusion. With your aid i found some clarity, but it seems i found it while you do the last review.

Due to unexpected rain, i made those dirty line. I have to save really quick and forgot to clean up, i will make sure it won't happen again. I will watch video that you mention, when i get lost again about git rebase.1. I am also not sure are you still busy on real world or not 2. Based on previous response time, i assume that this PR will get a bit of delay. Somehow i miss your last review on this PR.

I really felt bad about this, hope on tomorrow. It should bring fresh state.

Hey buddy, there's no need to feel bad about it. We're very happy that you want to contribute. Please don't worry about that 😃

Basically, when doing a rebase -i you will be prompted with a list of commits and a few actions to do with them.

Whenever you want to drop a commit you can just use the drop word in front of it.

In order to ammend a commit into the last one you can use fixup and you can also use squash to ammend commits and change their messages as well.

After doing it a few times you will see it's simple, you just gotta try, it's totally normal to make mistakes the first times you try it.

lucasfcosta · 2017-11-29T14:04:49Z

I got it now, I just had a minor comment about the value string.

Also, IMO we should change this API in the future to make it more consistent. I think that checking props on functions is a valid and not uncommon use case and therefore should be supported on this interface in the same way it is supported on the others.

Right now we need to pass a message that is truthy in order to do that in this interface.

sukrosono · 2017-12-13T17:07:51Z

😄 ahh a burst of happiness. Finally i got the point of rebase, also i can address my own misunderstanding.
Thanks for being patient 💘

I got minor curiosity, kindly someone answer it:

commit msg that i made via web interface (c1) is different with commit msg that i made on rebase (c2). Which thing is affected by c1 and c2?
what is saved on history is c2 right? what i mean is on git log

lucasfcosta · 2017-12-16T16:15:05Z

@brutalcrozt I'm afraid I won't be able to help regarding that since I don't use the web interface.

I'm also not sure I understand your question correctly.

But please make sure to check your git configurations. If you take a look at your commit you will be able to see that it contains an invalid-mail-address as one of the committers. Maybe this link can help you.

sukrosono · 2017-12-17T08:31:30Z

Nvm, the question is not really important.
Is it the reason you don't approve it?
As I remember the first commit is when I use live CD OS. I am in a rush so not configure the email properly.
I am not sure I know how to change the first commit email. Should I reopen new PR instead? 😉

lucasfcosta · 2017-12-26T14:26:50Z

@brutalcrozt please try this: https://stackoverflow.com/questions/3042437/change-commit-author-at-one-specific-commit

sukrosono · 2018-01-03T07:27:14Z

Okay, now it's green? 😍 . Thanks for believe in me.

lucasfcosta · 2018-01-03T22:08:38Z

Yay! You're almost there buddy, you just gotta change the commit message following these guidelines: https://conventionalcommits.org/

- on changes, decreases and increases family

sukrosono · 2018-01-07T02:41:09Z

done

meeber

Awesome work @brutalcrozt and @lucasfcosta. This LGTM.

astorije

Nice work! Will let @lucasfcosta review and merge if he's happy :)

meeber · 2018-01-17T20:05:51Z

@lucasfcosta Any chance you could take a look at this? You have an open review blocking merge :D

lucasfcosta · 2018-03-17T19:14:56Z

Oh, I'm so sorry I missed this for so long!

sukrosono requested a review from a team as a code owner November 9, 2017 10:38

sukrosono changed the title ~~test(assert), increase coverage~~ test(assert): increase coverage Nov 9, 2017

sukrosono mentioned this pull request Nov 18, 2017

Assert, getter function always fail with expecting a property #1090

Closed

sukrosono commented Nov 21, 2017

View reviewed changes

keithamus reviewed Nov 22, 2017

View reviewed changes

lucasfcosta requested changes Nov 22, 2017

View reviewed changes

lucasfcosta requested changes Nov 23, 2017

View reviewed changes

sukrosono force-pushed the patch-coverage branch from d3d5b24 to 13283b3 Compare November 23, 2017 18:00

sukrosono force-pushed the patch-coverage branch 2 times, most recently from 567ff6d to 36a455d Compare December 13, 2017 17:02

sukrosono force-pushed the patch-coverage branch from 36a455d to 7699a26 Compare January 3, 2018 07:24

test(assert): add spec with getterFn.

6eb1a50

- on changes, decreases and increases family

sukrosono force-pushed the patch-coverage branch from 7699a26 to 6eb1a50 Compare January 5, 2018 19:00

meeber approved these changes Jan 7, 2018

View reviewed changes

meeber mentioned this pull request Jan 14, 2018

chore: update dependencies #1118

Merged

astorije approved these changes Jan 15, 2018

View reviewed changes

lucasfcosta approved these changes Mar 17, 2018

View reviewed changes

lucasfcosta merged commit 11612e9 into chaijs:master Mar 17, 2018

snyk-bot mentioned this pull request Jan 29, 2020

[Snyk] Upgrade chai from 4.1.2 to 4.2.0 davemenninger/socket.io-chat-fargate#3

Open

snyk-bot mentioned this pull request Mar 15, 2020

[Snyk] Upgrade chai from 4.0.2 to 4.2.0 postmanlabs/sails-env-switch#15

Open

		assert.changesButNotBy(fnDec, obj, 'value', 1);
		// assert.changesButNotBy(fnDec, getterFn, 'value', 1);

		assert.changes(fn, obj, 'value');
		assert.changes(fn, getterFn, 'value');

test(assert): increase coverage #1085

test(assert): increase coverage #1085

Conversation

sukrosono commented Nov 9, 2017

codecov bot commented Nov 9, 2017 • edited Loading

Codecov Report

sukrosono commented Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasfcosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasfcosta Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasfcosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasfcosta commented Nov 29, 2017

sukrosono commented Dec 13, 2017 • edited Loading

lucasfcosta commented Dec 16, 2017

sukrosono commented Dec 17, 2017

lucasfcosta commented Dec 26, 2017

sukrosono commented Jan 3, 2018

lucasfcosta commented Jan 3, 2018

sukrosono commented Jan 7, 2018

meeber left a comment

Choose a reason for hiding this comment

astorije left a comment

Choose a reason for hiding this comment

meeber commented Jan 17, 2018

lucasfcosta commented Mar 17, 2018

codecov bot commented Nov 9, 2017 •

edited

Loading

sukrosono commented Nov 21, 2017 •

edited

Loading

lucasfcosta Nov 29, 2017 •

edited

Loading

sukrosono commented Dec 13, 2017 •

edited

Loading