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

Overhaul bdd documentation #920

Merged
merged 2 commits into from
Apr 11, 2017
Merged

Overhaul bdd documentation #920

merged 2 commits into from
Apr 11, 2017

Conversation

meeber
Copy link
Contributor

@meeber meeber commented Jan 27, 2017

Note to self: Rebase once #947 is merged and update docs for by since it now accepts a message argument.

There are two commits in this PR.

The first commit is a small change that renames the target argument to subject in a few assertions because "target" is an overloaded term which made it difficult to document.

The second commit is a huge overhaul of the bdd documentation intended to bring clarity, consistency, and guidance to every assertion. All of the examples that are intended to pass do so with the exception of two due to #919.

Closes #914
Closes #902

* should. With great power comes great responsibility. Whenever possible,
* it's best to assert that the one expected output was produced, rather than
* asserting that one of countless unexpected outputs wasn't produced. See
* individual assertions for specific guidance.
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

+1 great work.

* `property` assertions.
* Sets the `deep` flag, causing all `.equal`, `.include`, `.members`,
* `.keys`, and `.property` assertions that follow in the chain to perform
* comparisons using deep equality instead of strict (`===`) equality.
Copy link
Member

Choose a reason for hiding this comment

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

💭 It would be really nice here to link deep equality here rather than (or in addition to) the bottom of the description. Nice to have so won't block review on this.

* The 1st and 4th forms assert exactly what's expected of the output, whereas
* the 2nd and 3rd forms create uncertainty. Whenever possible, it's best to
* identify the exact output that's expected, and then write an assertion
* that only accepts that exact output as valid.
Copy link
Member

Choose a reason for hiding this comment

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

👍

* but may or may not have some of them.
*
* 4. `.not.any.keys`: Asserts that the target doesn't have any of the given
* keys.
Copy link
Member

Choose a reason for hiding this comment

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

💭 I wonder if there is a better way to phrase these? For my personal reading style these become blurry, hard to decipher. Perhaps examples would be better here? I wont block review for this though, it's much better than we currently have.

* later used by the `keys` assertion.
* Unsets the `any` flag, causing all `.keys` assertions that follow in the
* chain to revert to the default behavior of requiring that the target have
* all of the given keys, rather than only needing one or more of them.
Copy link
Member

Choose a reason for hiding this comment

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

🛑 Not sure I like this one. Feels like a long and winding sentence. Maybe better?

Unsets the any flag, causing any subsequent .keys assertions to revert back to normal behaviour. The normal behavior is that the target must have all keys, while any only expects some.

Also might be useful to document the interplay of any and all in the same chain. A rough draft of what I'm thinking:

Using .any.all is effectively a noop, you will rarely need to use both in one chain, but one example of this might be:
expect({ a: 1, b: 2 }).to.have.any.keys(['b', 'c']).but.not.have.all.keys([ 'd', 'e', 'f' ]);

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 we could have an example here too. This is kind of an "advanced" feature and I think many newcomers will be confused if they see this.

* expect(undefined).to.be.undefined; // Recommended
* expect(undefined).to.not.be.ok; // Not recommended
*
* A custom error message can be supplied as the second argument to `expect`.
Copy link
Member

Choose a reason for hiding this comment

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

👍 Makes me wonder if we should just straight up deprecate ok?

* In modern environments, this assertion is performed using the built-in ES6
* `Number.isNaN` method. In legacy environments, the target is compared to
* itself using strict (`===`) equality. The result is the same either way:
* the target is only considered to be `NaN` if it's exactly `NaN`.
Copy link
Member

Choose a reason for hiding this comment

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

🛑 I feel like this is not relevant to the user here. If there is a genuine edge case that users should be wary of, we should consider it a bug and fix the edge case.

* `undefined`. However, `.exist` should rarely be used. Whenever possible,
* it's best to assert that the target is equal to its expected value, rather
* than asserting that the target doesn't exist. See the `.equal` assertion
* for a better alternative.
Copy link
Member

Choose a reason for hiding this comment

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

💭 Again this pretty much feels like we should deprecate it.

* results if a bug is later introduced in your code that changes the target's
* type. See the `.a` assertion for details on how to test a target's type.
*
* expect([]).to.be.an('array').that.is.empty;
Copy link
Member

Choose a reason for hiding this comment

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

👍

*
* expect({}, 'nooo why fail??').to.be.arguments;
*
* The alias `.Arguments` can be used interchangeably with `.arguments`.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@keithamus
Copy link
Member

Quite a lot to get through here. I got up to arguments, will review the rest later. I've added some comments, but because this is so big I'll do some now and some later.

Key:

💭 - Just me expression thoughts, I won't block the review for these, but it might be nice to discuss these and maybe work to improve them, if you have time
🛑 - In my opinion, this should definitely change. This is why I have requested changes, fixing this will move my vote to approved
👍 - This is awesome work! Good job!

@meeber
Copy link
Contributor Author

meeber commented Jan 28, 2017

I'll hold off on commenting or making any changes until everyone (who wants to) has had a chance to perform a complete review.

One question though: Is it possible to add titled subsections to a particular assertion's docs? If so, how? My guess would be adding four pound signs like #### <subtitle>, but I don't know if that screws up how the website generation works in terms of headers and links in the sidebar?

* .but.not.have.ordered.members([2, 1]);
*
* If both the `contains` and `ordered` flags are set, the ordering begins
* with the first element in the target.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be the opposite? The ordering should follow the same order as the array given to the include assertion, right? I'm not sure it was just me that got it the wrong way but maybe this could be more clear.

* expect([ 1, 2, 3 ]).to.eql([ 1, 2, 3 ]);
* The `deep.equal` assertion is almost identical to `.eql` but with one
* difference: `deep.equal` sets the `deep` flag which could impact other
* assertions that follow in the chain.
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, that's interesting. Good job!

@@ -968,19 +1492,32 @@ module.exports = function (chai, _) {
});

/**
* ### .instanceof(constructor)
* ### .instanceof(constructor[, msg])
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could add some text detailing that this fails whenever not passed a valid constructor instance and which criteria are used to determine whether something is valid or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During my update, I didn't really see the need to get into the details of this failing due to an invalid constructor instance. I think it's sufficiently understood by the assertion itself that it only passes when the target is an instance of the constructor, without drawing any distinction between a valid-but-different-constructor and an invalid constructor in the case of failure.

* Due to a compatibility issue, the alias `.length` can't be chained directly
* off of an uninvoked method such as `.a`. Therefore, `.length` can't be used
* interchangeably with `.lengthOf` in every situation. It's recommended to
* always use `.lengthOf` instead of `.length`.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@lucasfcosta
Copy link
Member

Awesome job! I just left a few new minor comments.

I think this will be awesome for our contributors to read since it gives them so much information about how to use Chai effectively instead of just telling them how things work.

Great job! Also, @keithamus comments were great too, I agree with them all.

@meeber
Copy link
Contributor Author

meeber commented Feb 14, 2017

Note to self: Update .change, .increase, and .decrease docs to reflect change from chainable method to normal method.

* `.eql` can be negated by adding `.not` earlier in the assertion chain, but
* it should rarely be done. Whenever possible, it's best to assert that the
* target is deeply equal to its expected value, rather than not deeply equal
* to one of countless unexpected values.
Copy link
Member

Choose a reason for hiding this comment

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

👍

* number greater than the given number `n`. However, `.above` should rarely
* be used. Whenever possible, it's best to assert that the target is equal to
* its expected value, rather than asserting that it's greater than some
* value. See the `.equal` assertion for a better alternative.
Copy link
Member

Choose a reason for hiding this comment

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

🛑 "doLength flag" is an internal thing. We shouldn't expose this to users.

💭 I think saying "should rarely be used" is a little heavy handed. Perhaps we could reword it? Something like:

.above should only be used when you cannot guarantee an exact amount. Wherever possible, one should use .equal instead.

*
* expect(10).to.be.above(5);
* When the `doLength` flag is set, `.above` asserts that the value of the
Copy link
Member

Choose a reason for hiding this comment

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

🛑 Again, doLength is internals, let's write the assertion proper; with.length.above...

* expect([1, 2, 3]).to.have.lengthOf.above(2); // Not recommended
*
* `.above` can be negated by adding `.not` earlier in the assertion chain,
* but again, `.above` should rarely be used.
Copy link
Member

Choose a reason for hiding this comment

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

💭 Repeating ourselves here feels a little redundant. Mentioning best practices once per function doc feels sufficient.

* ### .least(value)
* ### .least(n[, msg])
*
* When the `doLength` flag isn't set, `.least` asserts that the target is a
Copy link
Member

Choose a reason for hiding this comment

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

🛑 Another case of doLength mentioned.

* expect([1, 2, 3], 'nooo why fail??').to.have.lengthOf(2);
*
* Instead of invoking `.lengthOf` as a method, it can be used as a language
* chain to set the `doLength` flag, causing all `.above`, `.below`, `.least`,
Copy link
Member

Choose a reason for hiding this comment

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

🛑 doLength is internals. User's need not worry, just demonstrate the chain:

lengthOf can be chained with other assertions - for example above, below, least...

*
* Asserts that the target is a number that's within a given +/- `delta` range
* of the given number `expected`. However, `.closeTo` should rarely be used.
* Whenever possible, it's best to assert that the target is equal to its
Copy link
Member

Choose a reason for hiding this comment

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

💭 another "rarely used" which is potentially hyperbolic.

* expect(1.5).to.be.closeTo(1, 1);
*
* `.closeTo` can be negated by adding `.not` earlier in the assertion chain,
* but again, `.closeTo` should rarely be used.
Copy link
Member

Choose a reason for hiding this comment

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

💭 more redundant "rarely used"

* given function `subject` returns a different value when it's invoked before
* the target function compared to when it's invoked afterward. Strict (`===`)
* equality is used to compare `subject`'s before and after return values.
* `.change` should typically only be used when it has been negated by adding
Copy link
Member

Choose a reason for hiding this comment

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

🛑 should be something like ".change - when used with one argument - should typically only be used with .not."

*
* expect(noop).to.not.change(myObj, 'dots');
*
* `.change` should rarely be used without negation. Whenever possible, it's
Copy link
Member

Choose a reason for hiding this comment

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

🛑 Should be something like ".change with two arguments should rarely..."

@keithamus
Copy link
Member

Okay @meeber I've reviewed all of this. Pretty much all great, I've got a few notes on some areas for concern though.

Largely I'm also sensing a theme - which is that these docs seem to suggest a lot of the assertions we provide have very limited use cases. Is this something we should be worried about? Taking a long look at the horizon, should we be slowly pushing these assertions into plugins?

@meeber
Copy link
Contributor Author

meeber commented Apr 5, 2017

Thanks for the review @keithamus. I know this was a tedious one to get through. I'll rebase once #947 is merged, go through all the feedback here, make changes, and update the PR.

Most assertions use the term "target" to refer to the object under
test, but a few assertions also had a `target` argument, resulting in
an overloaded term that was difficult to document. This commit
renames every `target` argument to `subject`.
@meeber
Copy link
Contributor Author

meeber commented Apr 9, 2017

@keithamus @lucasfcosta @vieiralucas @shvaikalesh I just updated this PR. I made several passes through the whole thing to address concerns, improve readability, soften and de-formalize some language, move reference links closer to concepts, etc. If there are any major issues, let's fix them. But if all that's left is minor issues, then I vote we merge and proceed with the next canary release.

* expect([{a: 1}]).to.deep.include({a: 1});
* expect([{a: 1}]).to.not.include({a: 1});
*
* // Target object deeply (but not strictly) includes `x: {a: 1}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing curly braces: {x: {a: 1}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's actually intentional. In the previous example, braces are included because .include is looking for an object {a: 1} inside the target array. But in that example, braces aren't included because .include is looking for a property x with a value of {a: 1} in the target object. I think it would take too much verbiage to clarify this distinction within the docs, without enough gain, so I vote just leave as-is.

@@ -92,18 +117,19 @@ module.exports = function (chai, _) {
/**
* ### .nested
*
* Sets the `nested` flag, later used by the `property` assertion.
* Enables dot- and bracket-notation in all `.property` and `.include`
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can link pathval here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure there's enough value with linking pathval. The value in linking deep-eql and type-detect is that they both have separate documentation that list rules and behaviors that are relevant to someone writing an assertion using those flags, but the documentation for pathval just provides documentation on a bunch of functions that are used when using the module independently of Chai. Since we're moving in the direction of having a bunch of micro libraries, I think we should only link those that provide direct relevant information to the Chai user.

* `.nested.property` special characters can be escaped by adding two slashes
* before the `.` or `[]`.
* If `.` or `[]` are part of an actual property name, they can be escaped by
* adding two backslashes before them.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: second backslash is necessary only to escape the first one, unless one does

String.raw`\.link`

If we are targeting beginner-level devs, wording is OK 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I vote to keep as-is for now to be beginner friendly, but it might be interesting in the future to add another example using the single-slash alternative.

@shvaikalesh
Copy link
Contributor

LGTM. Incredible job, @meeber! Love the consistency (wording, param names). Agreed on moving forward with merging.

Copy link
Member

@lucasfcosta lucasfcosta left a comment

Choose a reason for hiding this comment

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

I only thought about two possible changes, but nothing that could block this from being merged.
Awesome work.

* expect(1).to.equal(1);
* expect('foo').to.equal('foo');
*
* Add `.deep` earlier in the chain to use deep equality instead. See the
Copy link
Member

Choose a reason for hiding this comment

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

Many users get confused about the behavior of deep when it comes to object's prototypes, maybe it would be cool for us to explain more about it here and avoid issues later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think deep equality is mentioned too many times throughout the documentation to go into detail about the algorithm. I prefer linking to the deep-eql project page instead where those kinds of details can go.

* ### .least(n[, msg])
*
* Asserts that the target is a number greater than or equal to the given
* number `n`. However, it's often best to assert that the target is equal to
Copy link
Member

Choose a reason for hiding this comment

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

IMO it makes thinks strange to have a feature and then discourage users to use it unless we show them a good reason to do it.
Maybe we could add a simple explanation telling them that when they assert something is not <other_thing> this means that something can be Inifinity - 1 things, which is too loose.

What do you think? I'm not sure this would be too much or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's about a dozen assertions that fall into this category. I intend on making the argument post-4.x that a lot of them should be removed from core. For now I kinda took the strategy of gently steering users away from misusing these assertions by using language like "it's often best to do this other thing" and showing an example. For any such assertions that we end up keeping, I'd be in favor of adding a bit more explanation on when it is an appropriate time to use it.

Copy link
Member

Choose a reason for hiding this comment

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

@meeber I'm convinced.

LGTM.

@keithamus
Copy link
Member

2 approvals makes a merge 😄

Good work, as always @meeber

@keithamus keithamus merged commit 526f885 into chaijs:master Apr 11, 2017
@meeber meeber deleted the update-docs branch August 6, 2017 13:49
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