hoodiehq / hoodie-connection-status

:dog: connection status API for the browser
Apache License 2.0
5 stars 10 forks source link

Cover all the things (with tests) #33

Closed janecakemaster closed 8 years ago

janecakemaster commented 8 years ago

this PR brings coverage from 98% to 100% by writing extra unit tests for

close #4

janecakemaster commented 8 years ago

@gr2m the lines lacking coverage are all related to aborting the promise when a request exists. i've been trying to wrap my head around how to get into that state where a request has already been made for a test, but idk how to do that. any thoughts?

gr2m commented 8 years ago

Great work so far @janecakemaster :+1:

The best way to test methods in certain states are unit tests. For example, you could create a new unit test for the reset() method, where you create your own state just as you want it, e.g. like this:

var state = {
  method: 'HEAD',
  url: 'https://example.com/ping',
  cache: {},
  emitter: {
    emit: simple.stub()
  },
  request: {
    abort: simple.stub()
  }
}

and then pass that in directly

var reset = require('../../lib/reset')
reset(state)

That way you only test what is happening in this one method, everything else can be mocked.

You can give a try yourself. If you get stuck, here is how the tests/unit/reset.js file could look like: https://gist.github.com/3d1dda25434762103a85 – make sure to add it to tests/unit/index.js

Does that help?

janecakemaster commented 8 years ago

That part is clear to me, but what's not clear to me is how to make sure that promise.abort in the request module gets tested. if i stub it out in the test then those lines aren't covered. so far there is no way to actually fake a request currently in progress.

screen shot 2016-01-24 at 11 52 30
gr2m commented 8 years ago

looking into it

gr2m commented 8 years ago

@janecakemaster okay, so to test the abort method, we need to add an integration test, that sends a request and right away calls .reset() so that the request gets aborted before the response arrives.

This test should do, you can add it to test/integration/reset.js

test('connection.reset() aborts requests', function (t) {
  t.plan(4)

  var connectionStatus = new ConnectionStatus({
    url: 'https://abort-example.com/ping'
  })

  connectionStatus.check()

  .catch(function (error) {
    t.is(error.name, 'AbortError', '.check() rejects with AbortError')
    t.is(error.message, 'Aborted', '.check() rejects with message "Aborted"')
    t.is(error.code, 0, '.check() rejects with code 0')
  })

  connectionStatus.reset().then(function () {
    t.pass('reset resolves')
  })
})

Does that make sense? Please ask away if you have any questions. I remember myself being very confused about unit vs. integration tests et all

janecakemaster commented 8 years ago

thanks for the offline explanation @gr2m!

=============================== Coverage summary ===============================
Statements   : 100% ( 154/154 )
Branches     : 93.75% ( 45/48 )
Functions    : 100% ( 27/27 )
Lines        : 100% ( 154/154 )
===============================================================================

ready after your OK!

gr2m commented 8 years ago

It looks great :+1: we need to squash the commits. For lib/check.js, commit it as chore: typo, the rest we can commit as test: 100% coverage. Do you want to do it? Let me know if you need any help

janecakemaster commented 8 years ago

squashed nd ready to go

janecakemaster commented 8 years ago

gr2m commented 8 years ago

woop woop

varjmes commented 8 years ago

:tada:

NickColley commented 8 years ago

:100: :+1: :cake: :tada: