-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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.
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
ca: make test naming consistent #11713
Conversation
While working on the CA system it is important to be able to run all the tests related to the system, without having to wait for unrelated tests. There are many slow and unrelated tests in agent/consul, so we need some way to filter to only the relevant tests. This PR renames all the CA system related tests to start with either `TestCAMananger` for tests of internal operations that don't have RPC endpoint, or `TestConnectCA` for tests of RPC endpoints. This allows us to run all the test with: go test -run 'TestCAMananger|TestConnectCA' ./agent/consul The test naming follows an undocumented convention of naming tests as follows: Test[<struct name>_]<function name>[_<test case description>] I tried to always keep Primary/Secondary at the end of the description, and _Vault_ has to be in the middle because of our regex to run those tests as a separate CI job. You may notice some of the test names changed quite a bit. I did my best to identify the underlying method being tested, but I may have been slightly off in some cases.
@@ -462,3 +464,34 @@ func TestCADelegateWithState_GenerateCASignRequest(t *testing.T) { | |||
req := d.generateCASignRequest("A") | |||
require.Equal(t, "east", req.RequestDatacenter()) | |||
} | |||
|
|||
func TestCAManager_Initialize_Logging(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was moved from agent/consul/server_test.go
. It is mostly testing the CAManager, so this file seems more appropriate. I suspect we could move this one assertion into one of the existing Initialize
tests, but I've left that for another time.
I suspect we'll also want to move most of the leader_connect_test.go
tests to this file, but I also left that for another time to reduce conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOVE IT 😍
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/513997. |
ca: make test naming consistent
ca: make test naming consistent
While working on the CA system it is important to be able to run all the tests related to the system, without having to wait for unrelated tests. There are many slow and unrelated tests in agent/consul, so we need some way to filter to only the relevant tests.
This PR renames all the CA system related tests to start with either:
TestCAMananger
for tests of internal operations that don't have RPC endpoint, orTestConnectCA
for tests of RPC endpoints.This allows us to run all the test with:
The test naming follows an undocumented convention of naming tests as follows:
I tried to always keep Primary/Secondary at the end of the description, and
_Vault_
has to be in the middle because the regex used to run those tests as a separate CI job (ref).You may notice some of the test names changed quite a bit. I did my best to identify the underlying method being tested, but I may have been slightly off in some cases.
I think this test naming convention is also very important so that we know which tests exist (and which ones are missing). Sometimes it is better to fix or add assertions to an existing test, than to write a new one. And when making a code changes, it's important to be able to quickly see which cases are already well tested, and which are not.
I'm planning on backporting this to reduce conflicts as we make other changes.
I also renamed
InitializeCA
toInitialize
sinceCA
is already in the struct type name.