Update the docs for start_supervised(!)/2 to mention process linking

[whatyouhide]

We should mention in their documentation that start_supervised/2 and start_supervised!/2 do not link the child process to the test process.

Usually this is fine because of two reasons:

• The test process tends to interact with the child process, so the test process usually notices if the child process crashes
• If the child process crashes, that's logged; this doesn't show up only for short tests and race conditions and so on

However, sometimes you want to start a child process just to do some perform some side effects. In such cases, since the test process doesn't interact with the child process, it might be useful to manually link the two with Process.link/2.

Let's also add an example of such a process to contextualize the docs.

This idea was discussed in the elixir-lang-core mailing list.

[joladev]

I do suspect that people would expect start_supervised! to start linked processes, although changing the default behavior would be too much of a breaking change, which is why I suggested a start_link version (which fits well with the convention of start and start_link.

Usually this is fine because of two reasons:

* The test process tends to _interact_ with the child process, so the test process usually notices if the child process crashes

* If the child process crashes, that's logged; this doesn't show up only for short tests and race conditions and so on

I just have two issues with this I'd want to raise:

1. If you're using Mox/Hammox to mock and set expectations, you will not interact with the process directly and verify! just checks if the expectation was called, not if it was successful.
2. This is the bigger one: even if something is logged, the test passed. This has huge implications on tests in CI, where the test run will be successful even if it's logging errors.

By requiring users to manually link every time, there's a greater risk of people accidentally making mistakes and having broken tests that go unnoticed. I know adding a linked version of start_supervised seems silly when all you can do is add another line, but then you put the onus on the user to always remember to add it, or to understand ahead of time that their specific test requires it to be reliable.

Adding a recommendation to the docs is definitely a good idea though and I'm happy to be assigned/make a PR.

[whatyouhide]

@joladev the reason we want to start with just documentation changes is that you can have a linked child process and a test process that traps exits. This means that the test process doesn't crash if the child crashes, even if linked. The supervisor would restart the child and we would not link the restarted child back again to the test process.

For point 1., you mean that if the function passed to the expectation fails and it happens inside the child process, then it doesn't crash the test process?

[joladev]

@joladev the reason we want to start with just documentation changes is that you can have a linked child process and a test process that traps exits. This means that the test process doesn't crash if the child crashes, even if linked. The supervisor would restart the child and we would not link the restarted child back again to the test process.

That seems like a good thing to state in the docs for start_link_supervised. If the implementor has opted to trap exits in the test process I would expect them to also handle that. Same as if you do start_link (which is what some people opt for because they don't trust start_supervised) and trap exits, you have to manage that.

For point 1., you mean that if the function passed to the expectation fails and it happens inside the child process, then it doesn't crash the test process?

That is correct, or in the case of Hammox, where the type checks fail and the expectation raises. Today any kind of test that doesn't finish with an assertion that talks to the process would pass, even if the process crashed. Same for any scenario where the final assertion doesn't "catch" that the process under test has crashed, restarted, and is now back in a similar state. Imagine a scenario where you test incrementing a counter in the process, then test decrementing the counter, then finally checking the state of the counter. A crash on the counter decrement step would cause a passing final assert, because the counter is back to the initial state before the increment. Silly example but trying to illustrate.

Again to me it seems like linking is the saner default behavior, since you're reducing the risk of silent errors. This is not an option, so recommending a different approach seems like the way for it. Adding documentation to recommend people to link (keep in mind that most of the time people won't know they need to link, they would only realise after the find the issue with their test) is for sure an improvement!

[josevalim]

That is correct, or in the case of Hammox, where the type checks fail and the expectation raises. Today any kind of test that doesn't finish with an assertion that talks to the process would pass, even if the process crashed.

I don’t think the test needs to finish with an assertion, but you need to assert on the result of calling the process, even if you are using a mock. Perhaps we need more concrete examples but this particular example tells me the mistake is expecting that invoking a mock is enough for testing, which is almost never the case, as the return value still needs to find its way to the test process.

The same with a counter. If a sync operation fail, it should reflect on the result of the operation. If you are testing an async operation, then you need to assert on the side effect. But if restarting makes the same side effect visible, then perhaps the test needs to be more robust?

[joladev]

I don’t think the test needs to finish with an assertion, but you need to assert on the result of calling the process, even if you are using a mock. Perhaps we need more concrete examples but this particular example tells me the mistake is expecting that invoking a mock is enough for testing, which is almost never the case, as the return value still needs to find its way to the test process.

I'm not sure I agree that testing a process with mocks is not enough. If you've set expectations and assertions on the behaviors you want to see in the process, and those pass, you're good. Note that Hammox allows setting your assertions in the expect itself, because an incorrect call will raise (unfortunately raising in the other process and being ignored by the test) and a missing call will fail on verify!. With a linked process you get a reliable test, with an unlinked one you don't.

The same with a counter. If a sync operation fail, it should reflect on the result of the operation. If you are testing an async operation, then you need to assert on the side effect. But if restarting makes the same side effect visible, then perhaps the test needs to be more robust?

Again I believe that puts unnecessary onus on the coder to figure out in what ways the test is not robust, instead of them being set up to succeed. It's hard to know that you're making a mistake if your tools don't assist you. We had broken tests that we thought were fine because our tools didn't tell us they were actually crashing the process each time. With Process.link added the tests are now reliable, and we've started ensuring all our tests that use start_supervised also use Process.link, unless there's some specific reason why that can't be used. Not because each test explicitly requires it, but because it's a safer default that catches issues we didn't expect and gives better test output.

But if restarting makes the same side effect visible, then perhaps the test needs to be more robust?

Specifically here, even in this case, if you write your test so defensively that it would catch this, the test output would still not tell you about the crash (apart from the logs that come as a side effect, not visible if you've disabled logging to stdout in tests). The test output would say "expected 4, got 6", instead of showing the stacktrace of the crash you triggered. With Process.link you get a better behavior, even for a "more robust" test.

[josevalim]

I'm not sure I agree that testing a process with mocks is not enough.

To be clear, my point is that you need to also assert on the result of the code that calls a mock. That's because the code always goes like this:

test -> system under test -> mock

Which of course returns a value like this:

mock -> system under test -> test

So for you to assess the result of mock was correctly handled, you need to match on the result all the way up to the test. If you simply rely on Mox/Hammox, you test only the boundary between mock/system under test, but not that the system under test itself will fully handle that result.

[joladev]

Just want to flag that I haven't had the time to look at this and I'm going on vacation for a couple of weeks.

So for you to assess the result of mock was correctly handled, you need to match on the result all the way up to the test. If you simply rely on Mox/Hammox, you test only the boundary between mock/system under test, but not that the system under test itself will fully handle that result.

That's not necessarily true for a cast where the only thing happening in the handle_cast is calling the mock, ie all we want is to assert that the mock is correctly called (and that it didn't raise). In general you'll have that problem with handle_cast, unless you've coded your GenServer so that you can check the state with a separate function after the handle_cast has run.

Technically I guess it's true for all interactions with another process, you want to be able to assert that apart from the functions and behaviors you're checking, that it doesn't crash. Something Process.link gives you as a general guarantee, all other assertions are only as a side-effect checking that it hasn't crashed.

[josevalim]

Hi @joladev, no worries!

Technically I guess it's true for all interactions with another process, you want to be able to assert that apart from the functions and behaviors you're checking, that it doesn't crash.

Sure, but there are other ways to verify this: for example, by using monitors. Or by setting max_restarts to 0. Also, there are other things that could happen with a process, such as going into an infinite loop, even with a handle_cast, that won't be verified by linking them.

Overall, my main concern is not with the feature per se, but the underlying motivation to use it to write tests that simply assert a mock was called, which are ultimately brittle. If that's one of the main rationale for introducing this feature, then my preference is to argue that more robust tests are necessary. For example, in the handle_cast case, I would make it so the mocked code send a message back to the test process. The main use case of verify! is to raise if something was not called. Verifying if something was called is not much of a successful criteria.

If we go back to this:

If you're using Mox/Hammox to mock and set expectations, you will not interact with the process directly and verify! just checks if the expectation was called, not if it was successful.

A process not crashing is, the huge majority of times, not enough criteria to say a test was successful. It can be a criteria for it being unsuccessful though. So in order to move this conversation forward, I propose that we have actual code samples of tests that would benefit from being linked. If said tests can measure success (outside of "was this mock called?") and they could still have bugs that would only have been caught by being linked, then we can discuss how to make failure detection easier.

[joladev]

I don't mean to keep this thread going forever, I just feel like maybe I haven't expressed myself clearly, so trying to do that.

For sure we can look at examples, but let me just clarify that when I talk about mocks or handle_cast I'm giving examples, but the main reasoning behind wanting start_link_supervised! is not about specific behaviors, it's about recommending a safer default, one where you avoid the risk of a test accidentally passing even though the code failed, without putting the onus on you to write perfect tests or considering edge cases, while still having the ability to use start_supervised! where you are writing tests that want the process to crash (or don't care that the process crashed).

As in, I should be able to feel safe that if my process crashes unintentionally that that is guaranteed to fail the test and the failure reported as part of the test failure. This doesn't mean that the test doesn't also require different assertions. While if I want a test to kill the process intentionally without failing the test, I have the ability to write such a test (in that case I'd use start_supervised!).

Most test scenarios today that involve start_supervised! would benefit from using start_link_supervised! instead, not because it is strictly required, but because it gives stronger guarantees that the test framework would help you when you're making a mistake, and in many cases providing a better test failure message. The exception being scenarios where you don't actually want the process to be linked, in which case you can use start_supervised!, similar to how you decide whether to use start or start_link when you start a process in the first case.

Most people I've talked to about this don't realise that start_supervised! does not link, they would've expected it to. Some told me to just use start_link and ignore start_supervised! completely. This is anecdotal though, and I can't claim to have asked every member of the community!

[josevalim]

Alright, that alleviates my concerns. A PR for start_link_supervised! is welcome! :)

[josevalim]

Closing this issue as this issue per-se is outdated, the PR is welcome though!

[joladev]

Awesome, thank you!

[mat-hek]

Hi there, when a process started with start_link_supervised! terminates very quickly, the function fails with :noproc. For example:

  test "start link supervised" do
    start_link_supervised!({Task, fn -> :ok end})
  end

  1) test start link supervised (StartLinkSupervisedTest)
     test/start_link_supervised_test.exs:4
     ** (ErlangError) Erlang error: :noproc:

       * 1st argument: the pid does not refer to an existing process

     code: start_link_supervised!({Task, fn -> :ok end})
     stacktrace:
       :erlang.link(#PID<0.309.0>)
       (ex_unit 1.14.0) lib/ex_unit/callbacks.ex:562: ExUnit.Callbacks.start_link_supervised!/2
       test/start_link_supervised_test.exs:5: (test)

Is that the desired behaviour? I just discovered a race condition in my tests because of that - I guess a process sometimes terminates before being linked with Process.link. I think it's a bit counterintuitive because there are no such problems when using start_link directly.

[josevalim]

The expectation is for the test process to break, but the error message is not clear, although I don't think we can do something about it rather than document it. I pushed a commit that does so.

[mat-hek]

Thanks

The expectation is for the test process to break

Even if the started process returns normally?

[josevalim]

Ah, in this case no, but there is nothing we can do. :(

[mat-hek]

Hmm, what if we started the process from the test process directly and only told the supervisor to link to the started process?

[josevalim]

Then it is not being supervised accordingly :)

[mat-hek]

Yup, right 🤦 Do I understand properly that in 99% of cases the only benefit of the supervision in this case is to make sure that the process exits when the test exits? Maybe there could be another mechanism that would only guarantee that? I imagine it could avoid corner cases like this.