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

Up coverage result on coveralls #504

Closed
wants to merge 3 commits into from
Closed

Conversation

mazerte
Copy link

@mazerte mazerte commented Feb 15, 2014

Everything is in title.

@SBoudrias
Copy link
Member

Hi, your test need some work to be of better quality. First of all, review our testing guidelines.

it('#shift()') doesn't mean anything. Methods level belongs in describe blocks, and it block must contains the expected behavior. Example:

describe('#shift()', function () {
    it('return the first item in the conflict lists');
    it('remove the first item');
});

it('conflict status skip') is not a good name. It state a context, not what is expected. It should be it('does X when status is "skip"').

it('appendFiles should work with css file') is not a good name. Describe block should be #appendFiles(), and it('work with css files'). Never use should in an it block; it is just verbose and add no value.

Don't use Inquirer and event emitting during test. It currently adds a lot of unecessary overhead. Instead, mock the prompt method directly with Sinon.js and return dummy data.

Last thing, try putting less setup code in each it blocks. Prefer using beforeEach for setup (adding conflicts, etc).

@SBoudrias
Copy link
Member

@mazerte Any chance you can address the precedent comments?

@mazerte
Copy link
Author

mazerte commented Mar 10, 2014

I'm sorry Simom, I didn't have time to do this. But I can fix my unit test for follow your guidelines.

I don't undertand how to test Inquirer responses with Sinon.js. I follow your test method in Inquirer repository. https://github.com/SBoudrias/Inquirer.js/blob/master/test/specs/inquirer.js#L42

@SBoudrias
Copy link
Member

Yes, I test it this way in Inquirer because I test the Inquirer implementation. In the context of Yeoman, you test Yeoman, not Inquirer and the readline. As so, you only need to mock (override) the Adapter prompt method for your own mocked one. For example:

beforeEach(function () {
    this.promptReturns = { status: 'force' };
    mockAdapter.prompt = function (questions, cb) {
        cb(this.promptReturns);
    });
});

@sindresorhus
Copy link
Member

ping :)

@SBoudrias
Copy link
Member

@mazerte still around?

@sindresorhus
Copy link
Member

Closing for lack for lack of response. We would be happy to reopen if you get interested again though :)

@SBoudrias SBoudrias reopened this Jun 17, 2014
@SBoudrias
Copy link
Member

Leave it open, I'll take care of the fixes myself if we don't get them before I get there.

@sindresorhus
Copy link
Member

Can we get this landed or closed?

SBoudrias added a commit that referenced this pull request Sep 14, 2014
@SBoudrias
Copy link
Member

Ok, that's done - thanks for the initial work @mazerte, it's been very useful!

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.

3 participants