Blockscout: Request For Commment: doctests vs test

[KronicDeth]

Unlike RFCs for internet protocols, this is a literal request for comments, please leave comments describing how you want this to work.

From feedback in https://github.com/poanetwork/poa-explorer/pull/283, it appears that the various teams internal rules for when to doctest vs test differs. As we want to keep the overall project consistent, to allow different teams to work together and review each other codes and take community contributions, @amandasposito brought up that we should probably discuss this as an issue in GitHub instead of just in video chat. This will allow us to document our reasoning for the documentation/testing guidelines we adopt as a project, so that people in the future know what we thought of and rejected when making the guidelines, which hopefully will limit future questions and issuing being opened here on GitHub.

Current styles

I (@KronicDeth) write doctests whenever possible. This includes potential integration tests, such as insert calls to the Repo and EthereaumJSONRPC calls that depend on the Sokol chain. I find writing the prose and accompanying doctests help me make sure the API is easy to use and not overly complex as having too many conditional in prose is more obvious than in code most times. I mostly don't write doctests when they just won't work like for temporary GenServers or the complex asserts, like assert_raise or assert_receive as that would be very test-only code and not good examples to use in a dev iex shell.

@igorffs says he normally uses tests to explorer the behavior and then writes code and then documents last. So, much closer to the TDD ideal than my normal habit. I also write docs at the end sometimes, but it flows back and forth: if the doc writing points out a problem with the complexity, I'll rewrite and start documenting the new version.

@amandasposito similarly writes tests over doctests. [@amandasposito can you please expand if your style differs from @igorffs or if @igorffs was describing the Plataformatec team style overall)]

@Lucasnar came up with a good way of summarizing what should not be in a doctest - no tests that call outside services, so this would eliminate all doctests that use insert to setup database state or Explorer.Chain doctests that return non-empty state from the database. It would also eliminate doctests for EthereaumJSONRPC that depend on call to the Sokol chain. In general, this means doctests would have to be pure. This would be a similar guideline to how Ecto.Schema and Phoenix.View modules should be pure. This may eliminate the majority of the doctests, but it would ensure that we could tag all doctests converted to tests as integration tests for EthereumJSONRPC and optionally turn them off in certain test scenarios.

Prior discussion

@igorffs brought up having just examples, that weren't doctests, but I objected to that because I've had prior experience with long-lived (multiple year) open source projects, and it is incredibly easy for the examples to code rot with doctest checking the examples, so I think we shouldn't have examples that aren't doctests as the code rot leads to a frustrating user experience and either abandonment of the project when they can't get the examples or to work or angry GitHub issues about the docs not working.

I didn't mention it in the meeting but a caveat for the "no examples without doctests" rule would be for showing config and equivalent Application.put_env calls since we can't run those with doctest.

I proposed that if we adopt rules to ban the use of outside contact, like insert, and EthereumJSONRPC calls to Sokol from doctests or anything else which would affect whether the pre-existing doctests are no longer considered best practice for this project, as part of resolving this issue, I would go back through the code and convert the doctests to tests, so that we can record the guidelines that we agree to here in the README and the code would be following those guidelines, so that if developers copy the pre-existing code, it doesn't contain doctests that don't follow the new guidelines agreed upon here.

[josevalim]

I would just like to add general recommendations that should apply regardless of the project (IMO):

1. A doctest does not replace tests. Doctests are about documentation first and foremost.

2. Given doctests are about documentation, if you need to do overly complex setup to make a doctest pass, then it probably loses its goal as documentation. This rule is usually what restricts side-effects in doctests, because they tend to require setting up and cleaning up, but things like Repo operations are fine because the cleanup is handled elsewhere thanks to the sql sandbox (IMO)

3. If you write doctests (i.e. code examples in the documentation using the iex> syntax), then I strongly recommend that you also doctest TheModule. Otherwise someone editing the documentation may think that the test suite has their back, but it doesn't

[KronicDeth]

For (1), do you mean that we should duplicate behavior that is tested by a doctest in a test or just that we shouldn't have edge case tests in doctests unless that edge case needs to specifically described and demonstrated in the docs?

From (2), let's say that the EthereumJSONRPC tests that use Sokol can't be doctests because the fact that they require Sokol is hidden in the config files. Those should be rewritten to tests that explicitly set the url and trace_url Application environment to Sokol, so it is more explicit that we depend on Sokol. This will also protect the project when people use it on alternative chains, so they don't need to update the tests for their chain and have changes they can't PR back to this repo.

Also, from (2), the tests that use insert are still OK, except when they then use something that uses that inserted value to read Sokol.

I completely agree with (3).

[josevalim]

@KronicDeth Yes, you should "duplicate". I wouldn't consider it duplication, because they have two distinct purposes, but a doctest does not replace a test. I actually think that restricting tests to the doctest environment is harmful for the test itself, since we need to use the iex> syntax, we are restricted to a textual environment without syntax highlighting, assertions are more verbose, error reports are not as good, etc.