Finish existing tests [Phase 1] #12
Conversation
jgrosso
force-pushed
the
finish-existing-tests
branch
from
4a781ba to
592b02e
Compare
jgrosso
force-pushed
the
finish-existing-tests
branch
5 times, most recently
from
09fa557 to
6bffea9
Compare
On CI, `devDependencies` won't be installed, but Babel is required to `compile`, etc.
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.
jgrosso
force-pushed
the
finish-existing-tests
branch
from
6bffea9 to
2536e2b
Compare
| .then(() => core.push(`${remote} ${branch}`, { cwd: repo.path() }, callback)) | ||
| .then(({ stdout }) => ({ | ||
| ...generateResponse(), | ||
| push: generatePushStats(stdout), |
There was a problem hiding this comment.
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: '', |
There was a problem hiding this comment.
Do we not get an error message here that can be put in stderr?
| * ) => | ||
| * (chunk: Buffer) => | ||
| * string; | ||
| */ |
There was a problem hiding this comment.
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?
|
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. |
This consists of a lot of cleanup and some test infrastructure work.