Make style guide

[shaspitz]

Problem

We need a high level style guide for the repo with test suggestions, best practices, etc.

Note: this issue is more important as we approach launch, where code quality should be improving

Closing criteria

create a style guide based on scattered slack convos (consolidated below)

Example Content

Tests: Documenting and having intention in your test assertions is especially important in an async work environment, where context on someone else's work is not always easy to gain. Good practices to remember:

• Don't base a test on the already written implementation. Try to write unit tests with black box expectations if possible
• Smaller tests and functions are usually better and easier to follow
• Test logic should be readable down a file, avoiding multiple anonymous functions (esp with table driven tests)
• Make the "expected" and tested value clear for equal assertions. Expected values are best when hardcoded inline
• A high level overview of the entire test is always useful. What should the functionality be?

PRs: We move with a lot of speed adding changes to the ICS repo. Although speed of development is good, I would like to make sure that we do not rush (due to some feeling of emergency) and introduce bugs in the code

• Every PR must have two approvals to be merged
• Approving a PR means that you vouch and consequently are responsible for the changes in the PR. If you don’t feel confident to approve a PR, leave feedback as comments and ask somebody else to review
• We are indeed close to a deadline (~January), which means that low priority issues will not make the cut. Do not waste time with refactoring and tech-debt that are not security concerns (tech debt is to be considered on a case by case basis if it saves time before Jan). Open issues labeled accordingly and we’ll deal with them later
• Open PRs with proper names and description to keep track of the changes.
• Try to minimize the changes in each PR.
• PRs that are WIP, they should be open in draft mode.
• All the features implemented should include UTs / E2E tests.

[MSalopek]

Don't base a test on the already written implementation. Try to write unit tests with black box expectations if possible

Maybe we should define minimal test requirements and have it as a template. We could mark off all the criteria that were fulfilled.

The minimal set would force to you to think of edge cases, and not just the "happy path" that you know is working.

One example I can think of is testing that the system throws a panic when something is off (we have plenty of panic() calls in the code with no actual tests to see when they are invoked). That can be done via mocks - you can mock out a faulty response from a mock service and check for panics.

[MSalopek]

Also, it would be nice to have PR template.

The template should at least contain the following:

1. what is the purpose of the changes
2. what the changes are/how implementation was done (1-2 sentences)
3. how to test/how to confirm new behaviour

• steps for manual testing
• list integration/e2e tests if any were added

4. other developer notes

[danwt]

Nice writeup, I agree with most points.

Do not waste time with refactoring and tech-debt that are not security concerns (tech debt is to be considered on a case by case basis if it saves time before Jan). Open issues labeled accordingly and we’ll deal with them later

I'm a bit concerned about this idea. I know it's qualified ('to be considered...') but still - you could easily kill a software team taking this kind of fix-it-later approach IMO. I'd like to hear @jtremback word on this, and some real justification for support. IMO we could allocate a bit more time to improving livability of the codebase.

Rest of the points are good. I wonder if we should require 3 approvals for large/dangerous PR's.

[danwt]

I haven't read it myself but Ubers go style guide might be useful

• https://github.com/uber-go/guide/blob/master/style.md

[jtremback]

Nice writeup, I agree with most points.

Do not waste time with refactoring and tech-debt that are not security concerns (tech debt is to be considered on a case by case basis if it saves time before Jan). Open issues labeled accordingly and we’ll deal with them later

I'm a bit concerned about this idea. I know it's qualified ('to be considered...') but still - you could easily kill a software team taking this kind of fix-it-later approach IMO. I'd like to hear @jtremback word on this, and some real justification for support. IMO we could allocate a bit more time to improving livability of the codebase.

Rest of the points are good. I wonder if we should require 3 approvals for large/dangerous PR's.

We need to focus on shipping secure code. This currently consists of finishing remaining requirements and continuing to test. I think it will be a lot easier to refactor for clarity once the codebase stops moving so fast.

If there is tech debt that poses a security concern, make an issue, since that is high priority. If it is about readability/organization, please take time to fix it if you want and make a PR, but Github issues about this kind of thing will not be given a high priority.

And we already are spending a lot of time cleaning things up, Shawn in particular.

[danwt]

This is an interesting article

• https://a-nickels-worth.dev/papers/varnaming/