Review chapter 12 - QUnit

[wibblymat]

Shortest one yet:

Great intro to QUnit, but Backbone is not mentioned even once.

Edit I'll put my Sinon review here too, since chapter 13 is really also part of this chapter (looks like I messed up chapter splitting a little here):

This all looks great!

[dcmaf]

Adding my review comments for Chapter 12 - QUnit and Chapter 13 - SinonJS here:

Chapter 12 - QUnit

Section: Fixtures Example

The enumerate function defined in the example acts as a getter when called with no arguments and the template does not contain enumeration values, so wouldn't the expected value be zero?

Section: Asynchronous Code

Why won't the test hang if the success callback is never called (and thus start() is never invoked)?

test('An async test', function(){
   stop();
   expect( 1 );
   $.ajax({
        url: '/test',
        dataType: 'json',
        success: function( data ){
            deepEqual(data, {
               topic: 'hello',
               message: 'hi there!''
            });
            start();
        }
    });
});

Chapter 13 - SinonJS

Section: Stubs

• In the test code, shouldn't the id in the second test be '2'?

test('should find a model by id', function() {
    equal( this.todos.get(5).get('id'), 5 );
  });
});

• Test seems to be testing Backbone.Collection implementation of add() rather than application code. An application-based example would be more helpful.

Section: Mocks

Example needs description explaining what is going on in it.

Section: Exercise

The question-style comments with Hint references linking to Backbone documentation in the examples are inconsistent with the comment style in the rest of the book and the questions raised are answered within the book. Would recommend re-writing them as normal comments.

Section: Models

• Should note here that we are testing against the same Todo code presented in the Jasmine chapter (as opposed to the exercise earlier in the book).
• In the test code, wouldn't it make more sense to check that done is false and order is 0 in the first test, which is testing default values, rather than in the second?

module( 'About Backbone.Model');

test('Can be created with default values for its attributes.', function() {
    expect( 1 );

    var todo = new Todo();

    equal( todo.get('text'), '' );
});

test('Will set attributes on the model instance when created.', function() {
    expect( 3 );

    var todo = new Todo( { text: 'Get oil change for car.' } );

    equal( todo.get('text'), 'Get oil change for car.' );
    equal( todo.get('done'), false );
    equal( todo.get('order'), 0 );
});

• I changed the "error" event references to "invalid".

Section: Collections

These tests seem to be exercising Backbone functionality and not application logic. I would expect tests for the done(), remaining(), nextOrder(), and comparator() functions here.

Section: Views

• In second test, testing that 'done' is false would seem to be a test of the model rather than the view.

test('Is backed by a model instance, which provides the data.', function() {
    expect( 2 );
    notEqual( this.todoView.model, undefined );
    equal( this.todoView.model.get('done'), false );
});

• Async test could use more explanation of what is going on and when.
• I changed the last expect() in the async test to equal():

    $('#todoList li input.check').click();
    equal( this.todoView.model.get('done'), true );
});

Section: Event

These appear to be tests of the Backbone.Event implementation. Are there instances in which Backbone users would need to write event-related unit tests? If so, an example of that would be helpful; if not, then maybe remove this section.

Section: App

Example needs description explaining what is going on in it.

[addyosmani]

The question-style comments with Hint references linking to Backbone documentation in the examples are inconsistent with the comment style in the rest of the book and the questions raised are answered within the book. Would recommend re-writing them as normal comments.

@dcmaf I've dropped the refs to the external documentation, but was wondering - is your recommendation to drop the hints entirely? They are primarily there to provoke the reader into thinking about an area they have already read about, but I'm thinking of how better to phrase them. Would 'Hint: We covered this in chapter X, page Y' or something similar be more appropriate?

[dcmaf]

I think I found the style confusing since it is not consistent with the comment style in other examples in the book, which leaves me question why (e.g., is this a comment style commonly used in QUnit test cases). It is also confusing since the answer seems to be given by the following statement. For instance in:

test('Fires a custom event when the state changes.', function() {
    expect( 1 );

    var spy = this.spy();
    var todo = new Todo();

    todo.on( 'change', spy );
    // How would you update a property on the todo here?
    todo.set( { text: 'new text' } );

    ok( spy.calledOnce, 'A change event callback was correctly triggered' );
});

By this point, the reader certainly knows how to set a property on a todo, which is what is being done in the next statement. I would think it would be less confusing to simply state what is being done:

test('Fires a custom event when the state changes.', function() {
    expect( 1 );

    var spy = this.spy();
    var todo = new Todo();

    todo.on( 'change', spy );

    // Change the model state
    todo.set( { text: 'new text' } );

    ok( spy.calledOnce, 'A change event callback was correctly triggered' );
});

Also, looking at that same set of examples for Models, shouldn't the first test have an expect(3) and the second one an expect(1)?