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

Finish existing tests [Phase 1] #12

Open
wants to merge 42 commits into
base: master
Choose a base branch
from

Conversation

jgrosso
Copy link
Contributor

@jgrosso jgrosso commented Aug 31, 2017

This consists of a lot of cleanup and some test infrastructure work.

Joshua Grosso added 20 commits September 5, 2017 20:43
Tests rely on NodeGit calling the LFS filters when necessary, so we need to be using a version of NodeGit that supports this machinery in the first place.
Fix tests post-rebase
`git-lfs-pull`'s manpage says "Download all Git LFS files for current ref & checkout", and I don't see any options to pull a specific branch's files.
My guess is that this code was copy-and-pasted from the `fetch` command, explaining the extraneous argument.

Also, this commit has test stubs for the `fetch` and `pull` commands.
.then(() => core.push(`${remote} ${branch}`, { cwd: repo.path() }, callback))
.then(({ stdout }) => ({
...generateResponse(),
push: generatePushStats(stdout),
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you technically did not write this code, but it would be good to follow the same pattern as the other spawn based commands, where we look for response.push.push_error or whatever and if we find it we can set success to false and set the error message on stderr. Could you do that? See pull.js if you want an example, but I feel you know what I am talking about.

return {
...generateResponse(),
errno: BAD_CORE_RESPONSE,
stderr: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not get an error message here that can be put in stderr?

* ) =>
* (chunk: Buffer) =>
* string;
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you find it helpful to have the fake 'flow type' here? I am not necessarily opposed to it but we don't use flow in nodegit-lfs yet so it seems a little out of place. Maybe a nice comment or doc explaining what happens would go farther here?

@cjhoward92
Copy link
Contributor

I didn't really look through the tests. There are a lot. I might skim them, but since we effectively did not have tests before, I will count them as good when you address the comments above.

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.

2 participants