mozilla / frost

Unit testing framework for test driven security of AWS, GCP, Heroku and more.
https://mozilla.github.io/frost/
Mozilla Public License 2.0
104 stars 24 forks source link

[Meta] Test Rationale Use Cases & Implementation #387

Open hwine opened 4 years ago

hwine commented 4 years ago

In slack discussions and #383 & #384, we think of good usages for text to explain the "why" of a test (also called "rationale" in some places). However, there may be multiple implementations (mark.rationale and doc string), and there may be impacts on the desire to leverage existing Sphinx tooling to auto-document the code.

This issue is to capture use cases and discussion around these features before deciding on an implementation strategy. The current implementation strategies include:

Both the "rationale" and the "description" are output to the frost JSON for downstream use, but both are considered optional.

Action Needed

Each use case is a separate comment below -- folks can vote on whether or not they are "in scope" using reactions. Of course, folks should also add missing use cases, as well! We can then reach a decision ensuring the approach will support the remaining use cases.

Implementation concerns

The perceived tensions are:

hwine commented 4 years ago

UC-1

As a frost user, I want to understand which tests are appropriate for my run from the cli in order to minimize the chance of confusing the interpretation of the output.

hwine commented 4 years ago

UC-2

As an ops person doing triage on frost output, I want to minimize the chance of misinterpreting the results, to ensure the highest priority findings are mitigated first.

hwine commented 4 years ago

UC-3

As a frost configurator, I want to easily select which tests I wish to include in the profile I'm creating, so that I don't generate output of low value to my situation.

hwine commented 4 years ago

UC-4

As a frost developer, I want access to function definitions in the rendered documentation, so I don't misuse the API.

smarnach commented 4 years ago

I've always used the docstrings of test functions to document the rationale of the test. What else would I put in the docstring? A test function does not have any interface to document, since it's never explicitly called anywhere, so we can only document what it does and why. Using the docstring for this purpose doesn't feel like "overloading" the docstring to me, but rather like using it exactly for what it is intended.

Docstrings are included in the Sphinx documentation by default, and they are also easy to access programmatically for other purposes. Using pytest.mark.rationale() instead makes it more difficult to include the rationales in the Sphinx documentation, so it using docstrings looks easier to me.

AJ said on Slack that the original reason for using a pytest mark was that it was way easier to pass that string along into the results from a pytest run. Does this refere to this code?

https://github.com/mozilla/frost/blob/f9109d782b49bc8450480b91e956d1667019aaf6/service_report_generator.py#L133

If so, we could look into how difficult it would be to pass on the docstring instead.

hwine commented 4 years ago

Agreed on needing to look into the code more -- aiui, that location is "far away" from the actual test function, so introspection may be a challenge (but doable, I'm sure).

Let's simplify and defer this decision until we understand better what we want in the docs from #389. I think we have all the variant use cases on the table now, so we can make trade-offs knowledgeably.

smarnach commented 4 years ago

We could also go for some hybrid approach, e.g. by making the test loader copy the docstring into the rationale mark, or by providing a custom decorator for this purpose.

ajvb commented 4 years ago

I agree with figuring out the use-case(s) first, but just for future reference both the mark's and the docstrings are currently passed into the test results.

marks: https://github.com/mozilla/frost/blob/f9109d782b49bc8450480b91e956d1667019aaf6/conftest.py#L279

docstrings: https://github.com/mozilla/frost/blob/f9109d782b49bc8450480b91e956d1667019aaf6/conftest.py#L288