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

Refactor Tests #46

Closed
wants to merge 1 commit into from
Closed

Conversation

jamiebuilds
Copy link
Member

I started on some stuff and kinda went crazy.. I'll try my best to detail everything I did.

Changes:

The DEBUG mode tests were all different, so I flattened the describe/beforeEach blocks into just it blocks so it would read easier

this.reactStub        = this.sinon.stub(this.channel, 'react');
this.reactOnceStub    = this.sinon.stub(this.channel, 'reactOnce');
this.stopReactingStub = this.sinon.stub(this.channel, 'stopReacting');
this.commandStub      = this.sinon.stub(this.channel, 'command');
// instead:
this.channelStub = this.sinon.stub(this.channel);
// as long as you don't need to actually call anything on this.channel
this.callback = function() { return 'the thing'; };
this.callbackSpy = this.sinon.spy(this.callback);
// instead:
this.callbackStub = this.sinon.stub().returns('the thing');
// use these properly
this.sinon.spy(this.object, 'function_that_i_want_to_call_through');
this.sinon.stub(this.object, 'function_that_i_dont_want_to_call_through');
expect(this.channel._channelName).to.equal(this.channelName);
// instead:
expect(this.channel).to.have.property('_channelName', this.channelName);
this.channelName = this.channel._channelName;
this.channelKeys = _.keys(this.channel);
this.eventKeys   = _.keys(Backbone.Events);
this.requestKeys = _.keys(Backbone.Radio.Requests);
this.commandKeys = _.keys(Backbone.Radio.Commands);
this.eventIntersect   = _.intersection(this.eventKeys, this.channelKeys);
this.requestIntersect = _.intersection(this.requestKeys, this.channelKeys);
this.commandIntersect = _.intersection(this.commandKeys, this.channelKeys);
expect(this.eventIntersect.length).to.equal(this.eventKeys.length);
expect(this.requestIntersect.length).to.equal(this.requestKeys.length);
expect(this.commandIntersect.length).to.equal(this.commandKeys.length);
// instead:
expect(this.channel).to.contain(Backbone.Events);
expect(this.channel).to.contain(Backbone.Radio.Requests);
expect(this.channel).to.contain(Backbone.Radio.Commands);
expect(this.channelStub.listenTo).to.have.been.calledOnce;
expect(this.channelStub.listenTo).to.have.been.calledWithExactly('foo1', 'bar1');
// instead:
expect(this.channelStub.listenTo).to.have.been.calledOnce.and.calledWithExactly('foo1', 'bar1');
expect(function() { ... }).not.to.throw(Error);
// instead:
this.stopReacting = function() { ... };
expect(this.stopReacting).not.to.throw(Error);

You might just want to open the files side by side and go through them that way. The diff is gonna suck to go through

@jamiebuilds
Copy link
Member Author

@jmeas I'm so sorry

@jamesplease
Copy link
Member

Ha 👍 this is awesome. Thanks for doing this @thejameskyle :)

Reviewing now

it('should not log a console warning when firing a request on an object without a handler', function() {
this.Requests.request(this.eventName);
expect(this.consoleStub).to.not.have.been.called;
});
});
});
Copy link
Member

Choose a reason for hiding this comment

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

this is beautiful 😢

@jamesplease
Copy link
Member

👏

this is amazing @thejameskyle. Thanks again for doing this. I only had a few comments, then it's ready for merge I think.

@jamiebuilds
Copy link
Member Author

@jmeas I'll let you respond to my comments and I'll fix them all at once.

@jamesplease
Copy link
Member

I have no other comments, so feel free to update whenever you've got some time :)

@jamiebuilds
Copy link
Member Author

@jmeas Okay, updated

@jamesplease
Copy link
Member

Manually merged

@jamesplease jamesplease closed this Jun 3, 2014
@jamiebuilds jamiebuilds deleted the refactor-tests branch June 3, 2014 03:09
@jamiebuilds jamiebuilds self-assigned this Jul 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants