How to test non-zero return codes?

[gaggle]

I was surprised to see shellac throw an error when the command exits non-zero. I was expecting exit-code to be part of the normal object returned, because I don't think its shellac's business to decide what semantics apply to my exit codes.

To boil it down lets say I have these two tests:

  it(`should exit 1`, async () => {
    await expect(shellac.default`
      $ echo foo >&2; false
    `).rejects.toEqual(expect.objectContaining({ retCode: 1 }))
  })

  it(`should write to stderr`, async () => {
    await expect(shellac.default`
      $ echo foo >&2; false
    `).rejects.toEqual(expect.objectContaining({ stderr: "foo" }))
  })

The first test works fine. Yes I was surprised to see its a rejection, but it's simple to test the return code. The 2nd test fails though because as far as I can tell there's no stderr in the rejection object.

Does shellac offer a way to test stderr is handled correctly in this case? Maybe I've missed something..?

Other than that it's been nice to use the library! 👍 💪

(FWIW this is the actual code I'm using shellac for: https://github.com/gaggle/svgs-to-pdf/blob/main/test/svgs-to-pdf.spec.js)

[gaggle]

Maybe at the same time I can add that a failed command causes shellac to output "SHELLAC COMMAND FAILED!" sections which aren't helpful when I'm explicitly running negative tests.

[geelen]

Ooh ok yeah I hadn't considered how to handle this. It's super easy to extend the API tho, any thoughts on what might be a nice way to express what you're after? Maybe something like a fails { ... } ${ exitcode => ... }?

Edit: I do think that shellac should fail as it does (noisy messages and all) if you ever forget to wrap it in whatever block we come up with. It'd make tests for failure still "resolve" because your shellac script ran as expected, does that make sense?

[gaggle]

Yeah I think that makes sense! I say "think" because I might be confusing the return object vs. inline checking... you write the inline style but I guess I jumped entirely on the return object because that's what seemed most natural to my tests. So to be sure I understand you this is what I hear you suggest/imply:

  it("shellac should let me test a failing cmd exits 1 and prints to stderr", async () => {
    await expect(shellac.default.fails`
      $ echo foo >&2; false
    `).resolves.toEqual(expect.objectContaining({ exitcode: 1, stderr: "foo" }))
    // note here shellac resolves because `fails` method is used. No "noisy messages" are printed.
  })

  it("shellac should reject if the cmd DOESN'T fail when I say it fails", async () => {
    await expect(shellac.default.fails`
      $ echo foo; true
    `).rejects(expect.objectContaining({ retCode: 0 }))
    // here shellac rejects, because fails is used but the cmd doesn't fail. It will print "noisy messages" to guide the user.
  })

Something like that?

[geelen]

Nah I was more thinking about using it within a larger shellac block:

await shellac`
  $ echo "something normal"
  stdout >> ${ echo => expect(echo).toEqual("something normal") }

  fails {
    $ echo foo >&2; false
  } >> ${ (stdout, stderr, exitCode) => {
    expect(stdout).toNotContain("foo")
    expect(stderr).toContain("foo")
    expect(exitCode).toBe(1)
  }
`

That make sense? The name fails kinda doesn't work, but then I can't think of anything better...

[gaggle]

Ah okay, yeah super! I think that means I can use the resolved object like this:

it("shellac should let me test a failing cmd exits 1 and prints to stderr", async () => {
  await expect(shellac.default`
    fails {
      $ echo foo >&2; false
    }
  `).resolves.toEqual(expect.objectContaining({ exitcode: 1, stderr: "foo" }))
})

I also see your point on the fails naming but, yeah, it works for me. Is it possible to pass arguments to a shellac block? If it is then perhaps its worth considering if passing the exit code would be more precise, so I can tell shellac exactly what exit code to anticipate? It would be added precision to get shellac to reject when a command runs in a bad way, but you probably have a sense of what you think fits your vision best.

I think fails succinctly captures the essence of some non-0 exit code, and if you like the argument idea then perhaps exits(int) would have similar succinct connotations.

[geelen]

Yeah ok exits might work nicely. Is there a reason you want to use the return value of the whole shellac block using .resolves? I had in mind shellac being capable of the following syntax (adapted from your code)

  it("should merge svg file pattern to the specified pdf", async () => await shellac.default.in(dir.path)`
    $ ${runCmd} -o merged.pdf *.svg
    $ du -h *.pdf
    stdout >> ${ du => expect(du).toContain("4.0K\tmerged.pdf") }
  `)

Then, a failure case could be tested like this:

  it("should fail and show help", async () => await shellac.default.in(dir.path)`
      exits(1) {
        $ ${runCmd} ${arg}
      }
      stderr >> ${ stderr => expect(stderr).toEqual(expectedUsageText) }
  `)

I mean your usage should still work, but to me this reads a bit cleaner? I think you might be able to remove the async/await from the top line in both cases, since shellac always returns a promise, but I've kept it in there for clarity.

What do you think?

[gaggle]

I opted for the return-checking only because it's what I'm used to. I think I felt it was a bit more arrange/act/assert-ish, if that makes sense? But I don't actually mind and I've been considering changing to the inline so, yeah, maybe this is when I do it. I do agree your example reads a bit cleaner, so I'll at least give it a shot next I'm in the project.

[geelen]

Done. Merged in #5, released in v0.4.1 and documented in the README here: https://github.com/geelen/shellac#non-zero-exit-codes