tapjs / foreground-child

Run a child as if it's the foreground process. Give it stdio. Exit when it exits.

http://tapjs.github.io/foreground-child/

ISC License

39 stars 14 forks source link

coverage: add tests for arguments normalization #30

Closed demurgos closed 5 years ago

demurgos commented 5 years ago

Why

foreground-child has a complex signature but not all its variants are covered by tests.

What

Add tests for the signatures not covered by the other tests.

Other

Blocked on #25 (to fix a bug discovered by this PR)

demurgos commented 5 years ago

CI is failing because it uncovered a bug:

Assigning to cb also mutates the value in arguments so slicing the argumentss returns the overwritten cb instead of the process args. For example, fg("node", "foo", "bar", baz") yields ["foo", done => done(), "baz"]. This is a bug because the current argument normalization is clearly built in order to support this kind of calls.

Fixed in https://github.com/tapjs/foreground-child/pull/25

profnandaa commented 5 years ago

For this PR in particular, you might want to just duplicate the fix you did -- should work well after rebase.

For PR's that are blocked on #24, you can add a [wip]or [blocked] prefix on the titles until #24 is reviewed (and hopefully merged).

bcoe commented 5 years ago

@demurgos looks like a legitimate failure, let's get this sorted out before we land #25.

Otherwise I think this is looking good.

demurgos commented 5 years ago

@bcoe See my comment above: https://github.com/tapjs/foreground-child/pull/30#issuecomment-422700978

25 is actually fixing this bug.

bcoe commented 5 years ago

@demurgos mind rebasing and let's see if tests pass.

demurgos commented 5 years ago

@bcoe Rebased on top of #25. I also noticed that I left a debug log that was messing with the tests and removed it.