Inject exit command to Create in tests

[katafrakt]

There was an issue with test runs ending abruptly with no message when a database configuration was incorrect. In such cases the Create command ran an exit(1), which exited not only the command, but the whole test run.

The solution was already prepared, but not used: it was already possible to inject an alternative to just calling the actual Kernel.exit. This injects a test double instead and asserts it is not called.

Fixes #306

[cllns]

Excellent! Confirmed this works. Getting 317 examples evaluated every time locally now.

Can we get rid of the existing code that tried to solve this issue?

    # Prevent the command from exiting the spec run in the case of unexpected system call failures
    allow(command).to receive(:exit)

Or could we somehow make that work, since it's simpler?

I see that same line is used in the prepare_spec too, which isn't in this PR. Do we need to extend this fix to that file as well?

[katafrakt]

This existing code is actually in use, at least for Drop command (see here) - when I remove it, tests start exiting again. Perhaps Drop should follow the same pattern as other commands and call injected exit instead of just calling exit?

[cllns]

Ah, it looks like they come from the parent DB::Command.

Yea, I think we could inject the exit for that command. Another option would be switching to Process.exit throughout, then we could easily stub it on Process. The process exiting is a global state thing anyway, so I think it's fine to not inject it. WDYT?

In general, I wonder if we should be directly exiting at all this far down. It seems like it we could just raise a specific error for the situation and catch it higher up, and then exit in one place (in DB::Command, or even its parent App::Command). Thoughts? That would be a separate PR though, to be clear :) I don't want that to stop us from merging this and getting local CI working again.

As an aside, I don't think break exit code if code > 0 makes sense. exit raises a SystemExit so the break never gets called afaik. I think we can just remove break and it'll work the same.

[katafrakt]

I modified the Drop command to look and behave like the other commands in the PR. However, I still cannot remove the "old `allow", as tests start to fail.

I agree that just calling exit from the command class feel a bit aggressive and perhaps some other mechanism should be used, however maybe it's meant to be like that in Dry::CLI.

As for break, if the injected command does not raise SystemExit, it probably should break anyway, so I would keep it.

[timriley]

Thanks for this improvement, @katafrakt! The injected exit proc thing was something I had to introduce for one of those more complex DB commands that stitches together a range of other commands. It's a good change to be making this more consistent throughout.

Off the top of my head, I'm not sure why the allow(command).to receive(:exit) stubs still seem to be needed, but they're harmless for the time being.

I agree with your thoughts that calling exit directly from within a command feels like a blunt instrument! I'd like to change this in the future. Maybe a catch/throw kind of arrangement? It could be the kind of thing we eventually build into dry-cli, but worth trialling here inside hanami-cli first.