+
Skip to content

Add listForks #216

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

Closed
wants to merge 2 commits into from
Closed

Conversation

mushishi78
Copy link
Contributor

No description provided.

@AurelioDeRosa
Copy link
Member

Hi. Would you mind add a couple of lines in the README to document this method? Also, can you write a test for it?

@mushishi78
Copy link
Contributor Author

I added some documentation to the README, but I don't understand your testing so I'd rather leave that to you.

@aendra-rininsland
Copy link
Member

@mushishi78 Just out of curiosity, what test framework would be more familiar to you? We're rewriting all of our tests using something like Mocha, Jasmine or Qunit; any feedback is quite welcome!

@mushishi78
Copy link
Contributor Author

I'm more familiar with Mocha, but I also just didn't understand the tests. For instance, I looked at the test for listBranches as I assumed it would be similar:

    t.test('repo.listBranches', function(q) {
        repo.listBranches(function(err) {
            q.error(err, 'list branches');
            q.end();
        });
    });

Now I would have thought the if you listed the branches for this repo (michael/github) then you'd get 'master' and 'gh-pages' but you don't seem to be testing for that.

Plus, if I tested for all the forks of this repo, there would be like 349 different forks. I could then test if it got the first few, but that may change in the future with different people deciding to fork or remove their fork of the repo. So you'd need a test repo that'd been forked a few times, but most people wouldn't likely fork on their on volition.

@aendra-rininsland
Copy link
Member

@mushishi78 Ah, okay. Thanks, that's really helpful!

Currently, the calls to things like q.error(err, 'list branches') (Which is most of the assertions at present) are equivalent to something like should.not.exist(err) in Chai. Essentially, it's existence checking the error callback argument, and passing if it's undefined.

I'm rewriting all the unit tests in #248, any feedback is very much welcome! 😄

@aendra-rininsland
Copy link
Member

Closed in error; reopening.

@aendra-rininsland aendra-rininsland force-pushed the master branch 2 times, most recently from e746e5d to 6f04f13 Compare November 17, 2015 16:15
@AurelioDeRosa AurelioDeRosa self-assigned this Nov 28, 2015
@AurelioDeRosa
Copy link
Member

I've added a basic test for this PR. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载