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

Code Formatting and Testing #3

Merged
merged 6 commits into from
Dec 19, 2015
Merged

Code Formatting and Testing #3

merged 6 commits into from
Dec 19, 2015

Conversation

bkendall
Copy link
Contributor

I attempted to bring in all the thoughts on #1 and #2. Let's finish these off and get them into something official looking!

I attempted to bring in all the thoughts on #1 and #2. Let's finish these off and get them into something official looking!
- if it is on an object that is regenerated every test, no action is necessary
- if it is on a module or something persistent, always restore the function in an `after*`
- Stub modules and functions in the correct scopes (meaning, as close to the test that uses it as possible, but hierarchal stubbing is fine)
- When stubbing functions, we may stub the function to a 'truthy' behavior; doing so reduces code duplication, though the developer will need to adjust the stub(s) to test error cases
Copy link
Contributor

Choose a reason for hiding this comment

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

giving flexibility here leads to widely different looking test.

  1. setting stub functionality in it
describe('buzz unit test', function () {
  before(funtion () {
    stub(foo, 'bar')
    stub(foo, 'doo')
  })

  after(function () {
    foo.bar.restore()
    foo.doo.restore()
  })

  // below you would run through all permutations of foo.bar and foo.doo
  it('should fuzz', function() {
    foo.bar.returns(1)
    foo.doo.yieldsAsync('err')

    buzz()
    done()
  })
})

pros: in each test it is clear without scrolling anywhere, what each function inside the test should be doing
cons: lots of duplicate code for setting what stub do

  1. setting stub functionality in before's
describe('buzz unit test', function () {
  before(funtion () {
    stub(foo, 'bar')
    stub(foo, 'doo')
  })

  after(function () {
    foo.bar.restore()
    foo.doo.restore()
  })

  // below you make a describe for all permutations of boo.bar
  describe('foo.bar returns 1', function() {
    before(function(){
      foo.bar.returns(1)
    })

    it('should fuzz', function() {
      foo.doo.yieldsAsync('err')

      buzz()
      done()
    })
  })
})

pros: expected stub behavior is explained in the describe block
cons: more cruft (before,describe) code, have to scroll around the block to find out stub behavior

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 dislike the second example, but the first is much closer. Basically, by starting w/ the 'truthy' behavior, you can have:

  1. a test that just works, no extra stub adjustments
  2. a test for the first function failing
  3. a test for the second function failing
  4. a test for both functions failing

All of these translate pretty well to it statements in a test. I can definitely see a case where doing something similar to your second example may be of use (blanket assuming something works or doesn't, and then asserting the other things), but then I may tell you that your function is getting too complex if you get 3-4 different stubs that you're checking (that's 2^n tests that you have to create, remember)

does that make sense, or should I elaborate more? should the guide be tightened up to have more of an opinion towards stubbing for 'truth'?

Copy link
Contributor

Choose a reason for hiding this comment

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

my preferred method is way #1 as well. where you define what stubs do in each it statement. I think we should tighten this up to say
always define stub behavior in it statements

Copy link
Member

Choose a reason for hiding this comment

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

+1 for always define stub behavior initstatements. That seems reasonable to me. I used to do that way before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, here's the slightly more elaborate version. example first.

// this is describing an 'enter' function on a 'user'. 'enter' sets a value 'seat'
// on the user, which it gets from Bar.prototype.stool
describe('enter function', function () {
  before(function () {
    sinon.stub(Bar.prototype, 'stool').returns(7) // some 'success' case
  })
  after(function () {
    Bar.prototype.stool.restore() // restore the stub
  })

  it('should have the seat assigned to it', function () {
    user.enter()
    assert.equal(user.seat, 7, 'user has a seat')
  })

  it('should throw if no seats available', function () {
    Bar.prototype.stool.returns(-1)
    function _test () { user.enter() }
    expect(_test).to.throw(Error, 'no seat')
  })
})

basically, here's the goal: we want to be able to concisely test things, without having to read through a ton of stuff. By setting up Bar.prototype.stool to just return 7 up front, we don't have to worry about manually setting it every test (which would get tedious, and is prone to error - leaving behavior undefined is generally a bad idea). When you do want to define some non-correct or different behavior, you can change the behavior in your test (or in a before if you have a bunch, if you like), like where we do Bar.prototype.stool.returns(-1).

I think there's definitely some cases where a developer will want to group stubs and behaviors, which is why I left it a little vague. I think tending towards defining 'truthy' stubs first and changing them as needed is the way to go.

edit: formatting done messed up

Copy link
Member

Choose a reason for hiding this comment

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

I like @bkendall example. I didn't know before that you can stub function with default behavior once and then in one particular unit test change that behavior.
I would vote for this as a way to go


**Unit** tests are pretty obvious (or should be). They test exactly one unit of functionality. Most likely this should follow the boundaries of functions or at largest, modules. If we're finding we're writing a lot unit tests for a specific function, it may need to be broken up.

An important note: unit tests should _never_ touch an external service (e.g. database).
Copy link
Member

Choose a reason for hiding this comment

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

Don't almost all model test touch MongoDB? Is this something we aim to fix at some point? Is that an exception? What do we aim to do instead (for that particular case)?

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
Contributor Author

Choose a reason for hiding this comment

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

For the most part, @thejsj, you don't ever have to test that mongoose is doing the right thing (though I question it daily). Nothing there really needs to be tested. For the various other models where we are making requests through the database, we're usually manipulating data somehow and then sending that data into mongoose for it to do the work on the database.

In a unit test, mocking out the layer right at mongoose and simply returning sample objects is sufficient. If there's more paculiar logic that you may not be sure of or is more complex (I'm thinking like doing a map-reduce in mongo or making sure populate is doing the correct thing), you would move to an integration test and write it there.

The goal with unit tests is to be completely offline and not require any external dependency. Integration tests start to bring in external dependencies and functional tests will start the entire server/project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(if my comment and the readme don't align and I need to explain more in the readme, let me know. since it's something that's very 'in my head' it's hard for me to see if it's not clear enough in the readme)

Copy link
Member

Choose a reason for hiding this comment

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

I completely agree with you, but I fell you didn't address my question.

You're mentioning how everything should be and I'm asking what is going to be our strategy for all the stuff doesn't currently fit this model. I feel the README.md should be clear about what someone should do went they encounter a unit test that touches the DB (of which there are many).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I guess I didn't answer the second half...

Yes, those tests should eventually be fixed. However, this is a document saying how things should be, not what our goals are to address any issues we may have in our current tests. As far as I'm concerned, as people come in contact with sub-par tests as they are adding functionality or modifying them, any tests they touch should be updated accordingly. No one developer is going to have to (or want to) go through a 30-function-module and write tests for each one, but if one at a time is addressed, things will improve.

Copy link
Member

Choose a reason for hiding this comment

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

In a unit test, mocking out the layer right at mongoose and simply returning sample objects is sufficient. If there's more paculiar logic that you may not be sure of or is more complex (I'm thinking like doing a map-reduce in mongo or making sure populate is doing the correct thing), you would move to an integration test and write it there.

A version of this would be a good addition to the document, since I do think some guidelines on what should be a unit test vs an integration test is important. By that I mean, when do we actually need to touch the database (although most times we definitely don't!).

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've added a bit below.


There will always be discussion on where the lines between (for example) **unit** and **integration** tests. For a unit test in a mongoose environment, mocking out the layer right at mongoose and simply returning sample objects is sufficient. The developer should have faith (and understanding) that mongoose will correctly return objects. If there's more paculiar logic that you may not be sure of or is more complex (like a map-reduce or aggrigation in mongo, or making sure populate is doing the correct thing), you would move to an **integration** test and write it there.

These definitely aren't hard-and-fast lines in the sand. A good goal would be that no external services are required to get unit tests off the ground and running, but integration tests may need more setup (like a database). When we find various tests that don't fit into the classification they're housed in, we can always fix the dependency or move the test when appropriate.
Copy link
Member

Choose a reason for hiding this comment

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

👍 Nice!

bkendall added a commit that referenced this pull request Dec 19, 2015
@bkendall bkendall merged commit db5316e into master Dec 19, 2015
@bkendall bkendall deleted the format-testing branch December 19, 2015 01:57
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