Add missing tests based on coverage report

raszi / node-tmp

Temporary file and directory creator for node.js

MIT License

736 stars 92 forks source link

Add missing tests based on coverage report #164

Closed silkentrance closed 6 years ago

silkentrance commented 6 years ago

The coverage report from #107 shows that we are missing some tests.

This PR is about implementing additional tests so that we can increase our overall test coverage.

TODO

[x] invalid template option
[ ] valid template option with pre existing file or dir
[ ] TBD

silkentrance commented 6 years ago

@raszi never ever merge into existing PRs. this breaks the whole work flow and all the other PRs. You just increased the amount of work by a manifold. Yikes.

silkentrance commented 6 years ago

@raszi rebase is our friend here. rebase+merge gets the job done. and merge and then rebase will break everything, unless it is not done on the originating branch. AND NEVER THE GITHUB INTERNAL PR BRANCH, DAMNIT.

silkentrance commented 6 years ago

@raszi I do not know what you did to master but all my rebases here are failing with arbitrary merge conflicts and out of line/weird merge proposals. Did you push --force to master?

silkentrance commented 6 years ago

@raszi please use rebase wherever possible. I am a having a hard time with gh-121 and gh-155. while both have been resolved for now, merging these into master will take extra work...

silkentrance commented 6 years ago

wtf. is github bonkers or what? this cannot be merged, never.

silkentrance commented 6 years ago

@raszi for the record: I did not close this and i would have never merged into master. This leaves the question: who did close and merge this?

silkentrance commented 6 years ago

@raszi seems as if this was closed (again) due to my latest push --force attempt to fix existing issues in the branch. Yikes. Github is becoming somewhat intransparent right now.

raszi commented 6 years ago

@silkentrance I strongly disagree with your view and so does Linus Torvalds.

We should not fake the history and since this is not a local branch of yours and somebody else could use it therefore you should not rebase the changes.

Merging the recent changes is a task what you did and therefore it should appear in the history. You should rather merge the changes, resolve the conflicts and push the merge commit to the remote branch. GitHub will only show the differences and of course since it won't have conflicts the branch will be mergeable.

raszi commented 6 years ago

Long story short, you should only rebase your local changes and never rebase anything what you've already pushed to a remote branch.

raszi commented 6 years ago

This PR is borked for some reason. GitHub shows no commits and no changes. :worried:

raszi commented 6 years ago

You did a couple of force pushes, which could mess up GitHub. I tried to recover the PR from my local git repository, see #165