Skip to content

style: test determinism and doxygen additions to guide. #870

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattklein123 merged 3 commits into from
May 2, 2017
Merged

style: test determinism and doxygen additions to guide. #870

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b30eeec
style: test determinism and doxygen additions to guide.
htuch May 1, 2017
0c47abc
Fix mock links.
htuch May 1, 2017
67e9775
Weaken stance on internal comments.
htuch May 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions STYLE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@
fiasco](https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use) for
how to best handle this.
* API-level comments should follow normal Doxygen conventions. Use `@param` to describe
parameters, `@return <return-type>` for return values.
parameters, `@return <return-type>` for return values. Internal comments for

@dnoe dnoe May 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll register my mild opposition to this here. It seems if we are going through the work to document internal methods and member variables then we might as well follow the Doxygen style. This is largely an academic point for member variables but for methods doxygen provides a structured way of documenting arguments and return semantics that is just as relevant for private methods. The default Doxygen config can be set up to ignore private docs, but for someone new to the code base being able to run Doxygen with the call graph feature and all private documentation enabled seems a good way to create browsable overview documentation (I've used this in past projects, but admittedly we had more extensive documentation of private data and methods from the beginning).

@mattklein123 mattklein123 May 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion here. I guess I would say that in a perfect world, all methods, etc. would have doxygen comments. Realistically though, it's hard enough to get people doing it for public methods and class comments, and I think in most cases people will just not comment on private methods/members instead of going through the whole doxygen dance. So, giving into the reality of human nature I would tend to agree with @htuch. But happy to get some more opinions.

@dnoe dnoe May 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't necessarily think it should be mandatory to have doxygen comments for internal members/methods, just that if you're going to write such a comment it shouldn't be deliberately formatted to be ignored by Doxygen.

@htuch htuch May 1, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does increase the activation energy to comment by forcing people to document every arg/return. I might want to add a quick:

// foo does bar
int foo(char arg);

comment, I don't necessarily want to add full docs for return/args, but the documentation is better than nothing.

@mattklein123 mattklein123 May 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

@dnoe dnoe May 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd still prefer that we just relax the requirement for "going full doxygen" for private docs rather than mandating that private member/method have comments that Doxygen must ignore. Even in your example "foo does bar" is a perfectly reasonable comment for Doxygen to pick up as an internal method, extensive return type documentation isn't mandatory. In other words: Suggest but not require that private members/methods have a doxygen style comment, and make no hard mandates about all doxygen metadata being populated.

But, I like documenting things pretty extensively as part of my mental code writing process so that's my bias...

@mattklein123 mattklein123 May 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I can't say that I really have a strong opinion what to codify here. I will let the two of you sort it out. :)

@htuch htuch May 1, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. We've agreed that comment style for internal docs doesn't matter, but there should be some thought put into making the comment useful (e.g. talking about constraints on arguments etc.) when they are put there.

@dnoe will own adding proper Doxygen support to the build or stop using Doxygen in his internal comments ;)

@dnoe dnoe May 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe both of those things...

methods and member variables should be regular C++ `//` comments.
* Header guards should use `#pragma once`.
* There are probably a few other things missing from this list. We will add them as they
are brought to our attention.
Expand Down Expand Up @@ -91,7 +92,7 @@ A few general notes on our error handling philosophy:
`RELEASE_ASSERT` as the uncommon case, but experience and judgment may dictate a particular approach
depending on the situation.

# Hermetic tests
# Hermetic and deterministic tests

Tests should be hermetic, i.e. have all dependencies explicitly captured and not depend on the local
environment. In general, there should be no non-local network access. In addition:
Expand All @@ -111,6 +112,11 @@ environment. In general, there should be no non-local network access. In additio
`{{ test_udsdir }}` is provided for pathname based Unix Domain Sockets, which must fit within a
108 character limit on Linux, a property that might not hold for `{{ test_tmpdir }}`.

Tests should be deterministic. They should not rely on randomness or details
such as the current time. Instead, mocks such as
[`MockRandomGenerator`](test/mocks/runtime/mocks.h) and
[`Mock*TimeSource`](test/mocks/common.h) should be used.

# Google style guides for other languages

* [Python](https://google.github.io/styleguide/pyguide.html)
Expand Down