Add methodForTest() convention to the style guide.#1240
Add methodForTest() convention to the style guide.#1240mattklein123 merged 2 commits intoenvoyproxy:masterfrom
Conversation
This naming convention is used throughout Envoy but I didn't see any reference to it in the style guide.
STYLE.md
Outdated
| Envoy namespace there should be a comment of the form `// NOLINT(namespace-envoy)` at | ||
| the top of the file. | ||
| * If a public method is only intended to be called from test code then it should | ||
| have a name that ends in ForTest() such as aMethodForTest(). |
There was a problem hiding this comment.
Can we also add some kind of clause that this should be a last resort thing. IMO it's rare this is really necessary in prod code.
There was a problem hiding this comment.
I didn't call it out here explicitly but my understanding is that in Envoy style this is preferable to doing something with friend-ing the test code. Thoughts on that?
There was a problem hiding this comment.
I don't really have a strong opinion either way. My point mainly is that I think in practice it is actually quite rare that one needs to add a public method just for testing or friend the test code. Tests can frequently be written without doing any of that which IMO makes the tests better as the don't see into the inner details of the code under test. I would point that out, but I don't feel very strongly about it either way.
STYLE.md
Outdated
| Envoy namespace there should be a comment of the form `// NOLINT(namespace-envoy)` at | ||
| the top of the file. | ||
| * If a public method is only intended to be called from test code then it should | ||
| have a name that ends in ForTest() such as aMethodForTest(). |
There was a problem hiding this comment.
Thanks for adding this convention!
Is this also true for protected methods? Would "If a method defined outside of the test directory is only intended..." be more clear or too wordy?
There was a problem hiding this comment.
I think that's a better way to say it, I'll update.
Description: Because there was no separate callbacks struct, `c_on_engine_running` and `c_on_exit` were namespaced under `Engine`. This PR moves the `std::function` closures to a dedicated `EngineCallbacks` and moves `c_on_engine_running` and `c_on_exit` into an anonymous namespace. This PR also normalizes the callback structure for both `EngineCallbacks` and `StreamCallbacks`, so they're constructed & passed around the same. Risk Level: Low, none of this code is built into a production target. Testing: Pending Signed-off-by: Cerek Hillen <chillen@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description: Because there was no separate callbacks struct, `c_on_engine_running` and `c_on_exit` were namespaced under `Engine`. This PR moves the `std::function` closures to a dedicated `EngineCallbacks` and moves `c_on_engine_running` and `c_on_exit` into an anonymous namespace. This PR also normalizes the callback structure for both `EngineCallbacks` and `StreamCallbacks`, so they're constructed & passed around the same. Risk Level: Low, none of this code is built into a production target. Testing: Pending Signed-off-by: Cerek Hillen <chillen@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>
**Description** This adds embeddings tracing per OpenInference OpenAI semantics, and adjusts docs accordingly. **Related Issues/PRs** Fixes #1085 --------- Signed-off-by: Adrian Cole <adrian@tetrate.io>
This naming convention is used throughout Envoy but I didn't see any
reference to it in the style guide.