Testing: Parameter input validation

In the following code I was wondering if it's okay to adjust the tests that do the name checking. Because once I add the code that checks that the ID cannot be null my first 3 tests fail.

The same goes for the last 3 tests that do the id testing. I have to use 'foo' as a name cause otherwise those tests will fail too. Not sure if that's okay either?

Code to be tested:

class Error

class Node
    constructor: (name, id) ->
        if not name?
            throw new Error('Name cannot be null')

        @name = name

        if not id?
            throw new Error('Id cannot be null')

        @id = id

window.Node = Node

The spec:

node_spec = describe 'Node', () ->

    it 'should have the name foo', () ->
        expect( new Node('foo').name ).toEqual('foo')

    it 'should have the name bar', () ->
        expect( new Node('bar').name ).toEqual('bar')

    it 'should throw an error if the name is null', () ->
        expect( () -> new Node() ).toThrow()

    it 'should have an id of 0', () ->
        expect( new Node('foo', 0).id ).toEqual(0)

    it 'should have an of 1', () ->
        expect( new Node('foo开发者_如何学C', 1).id ).toEqual(1)

    it 'should throw an error if id is null', () ->
        expect( new Node('foo') ).toThrow()

window.node_spec = node_spec

Update: I guess one solution is to have a getter and setter methods for both id and name and test those instead.

Edited after matyr's comment below: I'd initially believed that this was due to the quirkiness of the new operator, since several of the tests use the syntax new Foo().bar. However, new behaves as Pickels had expected in those circumstances. So although this answer is not the solution to the stated problem, here's a review for posterity's sake:

When you write

new A().B

you're referencing the B property of a new A instance.

But if you write

new A.B()

(or just new A.B—CoffeeScript implicitly adds parentheses at the end of the new target if they're absent) then you're creating a new instance of A.B. This gets pretty confusing—what's new A().B()?—because the new operator has its own special precedence rules. So, I recommend using parentheses to clarify such code, e.g.

(new A).B
new (A.B)

There was discussion here about trying to make this behavior more intuitive in CoffeeScript, but after an energetic effort, it was determined that "the cure was worse than the disease."

Let me try this again: Right now, you have three tests that are failing. Let's figure out how to address each failure.

Test #1

expect( new Node('foo').name ).toEqual('foo')

This is getting an error because id is missing. I would keep the test for that error,

expect( new Node('foo') ).toThrow()

and rewrite test #1 to declare a legit, error-free Node instance, just as you do with the id test lines:

expect( new Node('foo', 1).name ).toEqual('foo')

There's nothing wrong with that. Every time you're testing for non-error behavior, you should be instantiating properly; in this case, that means passing a name and an id.

Test #2

expect( new Node('bar').name ).toEqual('bar')

This is basically the same as test #1. In fact, I'd say you're actually testing too thoroughly here. The question you should be asking is: Do I have reason to believe that passing 'bar' as a name instead of 'foo' will produce a different behavior? I'd say "no."

So I'd either remove the 'bar' test outright, or replace 'bar' with '' (assuming that you want to allow empty strings as names), since a misplaced or instead of ?, or if name instead of if name?, could cause '' to behave differently than a string of length > 0.

Test #3

expect( () -> new Node() ).toThrow()

This test is failing because () -> new Node() defines a function—a function which is never run. I think you mean to write just new Node() instead.

Other thoughts

Looking at the Speks docs, it sounds like a preferred style is cut down on repeated instance declaration code using beforeEach. Of course, this won't work when you're testing the constructor itself, but you may want to use it for the bulk of your tests in the future.

Here's a rewritten version of your test suite incorporating all of these suggestions and dividing the tests into three types: Those where an exception is created, those where normal behavior is expected, and those where the object should behave properly but is an edge case:

node_spec = describe 'Node', () ->

  # exception tests
  it 'should throw an error if there are no arguments', () ->
      expect( new Node() ).toThrow()

  it 'should throw an error if there is only one argument', () ->
      expect( new Node('foo') ).toThrow()

  # normal tests
  node = new Node('foo', 1)

  it 'should have an id of 1', () ->
      expect( node.id ).toEqual(1)

  it 'should have the name foo', () ->
      expect( node.name ).toEqual('foo')

  # slightly unusual case of id = 0, name = '' (convertible to false)
  node = new Node('', 0)

  it 'should have an id of 0', () ->
      expect( node.id ).toEqual(0)

  it 'should have an empty string as its name', () ->
      expect( node.name ).toEqual('')