Merge test guides into a single task-based guide by roaks3 · Pull Request #8307 · GoogleCloudPlatform/magic-modules

roaks3 · 2023-07-11T15:05:55Z

Subset of b/261212591

Fixes hashicorp/terraform-provider-google#12792
Fixes hashicorp/terraform-provider-google#12424

This change is similar to #8193, where we are making an effort to consolidate our guides into a low number of task-based guides.

The new guide walks the user through creating all necessary tests for a resource ("basic", "full", and "update"). There are tabs for MMv1 resources vs handwritten resources. The previous guides have been deleted, and any reference material will be consolidated into separate pages.

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Generated Terraform providers, and ran make test and make lint in the generated providers to ensure it passes unit and linter tests.
Ran relevant acceptance tests using my own Google Cloud project and credentials (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read Write release notes before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

modular-magician · 2023-07-11T15:20:37Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

modular-magician · 2023-07-11T16:37:59Z

Tests analytics

Total tests: 2848
Passed tests 2551
Skipped tests: 295
Affected tests: 2

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccComputeFirewallPolicyRule_multipleRules

Get to know how VCR tests work

modular-magician · 2023-07-11T16:40:47Z

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerAwsNodePool_BetaBasicHandWritten[Error message] [Debug log]
TestAccComputeFirewallPolicyRule_multipleRules[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

melinath

Overall this is a big improvement even though it doesn't cover everything in the ticket - leaving it at the current scope sounds good. I have some comments / questions / change requests below.

melinath · 2023-07-11T18:00:34Z

+{{< /tab >}}
+{{< /tabs >}}
+
+## Add an update test


As written, I find the procedures in this a little hard to scan; I'm not entirely sure how to fix it. I wonder if this might be a case where some additional context about the flow of the test would be useful?

Part of what makes it hard to scan is that there are a number of * placeholders here which mean different things from each other, which seems potentially confusing - see https://developers.google.com/style/placeholders for recommendations on how to use placeholders (not all of which apply but most of which are useful!)

I might have to circle back to this comment after going through the other items. Could you clarify what you mean by scanning, I'm not sure I understand the use case?

by "scan" I just mean "look at and be able to quickly identify / understand important pieces of information (for example because they are formatted consistently)"

Hmm ok, I think that makes sense. It sounds like you are finding the * to be inconsistent with the other documentation, and would prefer a placeholder instead? A placeholder gets really weird in these types of instructions, and I'm not fully seeing how it improves on the scan-ability you mentioned, but I can try it out if you think that would fix the issue.

coming back to this - I think I was thinking something like resource_PRODUCT_RESOURCE_generated_test.go. But I think I would also be fine with considering this non-blocking and coming back to it later if it's an issue.

I've taken another pass at this, and think it has turned out better, but curious your thoughts.

Instead of having so many editing instructions in a row without providing a concrete starting or ending place, I added examples of the result. I think this helps give the reader a model of what is being changed (two main blocks of code), and breaks it up visually. It can probably still be tweaked, and we may even be able to trim down the instructions further because the reader has an example to compare to.

Regarding the placeholders, we can still fit them in here, but I did come across a piece that I found weird, which is capitalization. If we used placeholders, I would expect something like resource_PRODUCT_RESOURCE_generated_test.go and then further down testAccCheckPRODUCT_RESOURCEDestroyProducer, and besides the format looking weird, PRODUCT_RESOURCE is not the exact same value in both cases. Can you think of a good way to handle this?

I think the examples are a good improvement and address a lot of my concerns. regarding the placeholders I was thinking you could do something like testAccCheckProductResourceDestroyProducer to match the camelCase but the examples might already sufficiently explain that part.

Ok, sounds good. FWIW I think that is a reasonable solution for the destroy producer, ideally if we can get the ProductResource part to clearly stand out, but I would be slightly worried about a reader thinking they need to delete a function with that exact name.

I will leave it with the asterisks for now as "good enough", but not opposed to future improvements.

modular-magician · 2023-08-01T14:05:58Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

modular-magician · 2023-08-01T14:26:19Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

modular-magician · 2023-08-01T15:48:21Z

Tests analytics

Total tests: 2910
Passed tests 2607
Skipped tests: 302
Affected tests: 1

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccContainerAwsNodePool_BetaBasicHandWritten

Get to know how VCR tests work

modular-magician · 2023-08-01T15:48:57Z

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerAwsNodePool_BetaBasicHandWritten[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

modular-magician · 2023-08-04T16:21:26Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

melinath · 2023-08-04T16:55:36Z

+{{< /tab >}}
+{{< /tabs >}}
+
+## Add an update test


I think the examples are a good improvement and address a lot of my concerns. regarding the placeholders I was thinking you could do something like testAccCheckProductResourceDestroyProducer to match the camelCase but the examples might already sufficiently explain that part.

melinath · 2023-08-04T17:05:18Z

+         subnetwork_name: "example-subnet"
+         network_name: "example-network"
+   ```
+   {{< hint warning >}}


for some reason this is rendering (for me locally) as

It looks like removing the indentation means that the words get placed correctly inside the colored area - it's maybe not ideal to not have the indentation but possibly the best we can do for now.

Yea, it's something that only happens inside the "tab", and I wasn't able to trick it into placing the text where I wanted. I'll just un-indent for now.

modular-magician · 2023-08-04T17:44:13Z

Tests analytics

Total tests: 2930
Passed tests 2628
Skipped tests: 302
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

modular-magician · 2023-08-04T21:15:16Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

melinath · 2023-08-04T21:16:27Z

+   - If beta-only fields are being tested, do the following:
+     - Change the file suffix to `.go.erb`.
+     - Add `<% autogen_exception -%>` to the top of the file.
+     - Wrap the beta-only tests in a version guard: `<% unless version = 'ga' -%>...<% else -%>...<% end -%>`.


Suggested change

- Wrap the beta-only tests in a version guard: `<% unless version = 'ga' -%>...<% else -%>...<% end -%>`.

- Wrap the beta-only tests in a version guard: `<% unless version == 'ga' -%>...<% else -%>...<% end -%>`.

melinath · 2023-08-04T21:16:49Z

+   - If beta-only fields are being tested, do the following:
+     - Change the file suffix to `.go.erb`.
+     - Add `<% autogen_exception -%>` to the top of the file.
+     - Wrap the beta-only tests in a version guard: `<% unless version = 'ga' -%>...<% else -%>...<% end -%>`.


Suggested change

- Wrap the beta-only tests in a version guard: `<% unless version = 'ga' -%>...<% else -%>...<% end -%>`.

- Wrap the beta-only tests in a version guard: `<% unless version == 'ga' -%>...<% else -%>...<% end -%>`.

melinath · 2023-08-04T21:17:00Z

     ```
     <% autogen_exception -%>
     ```
   - If any of the added Go code (including any imports) is beta-only, change the file suffix to `.go.erb` and wrap the beta-only code in a version guard: `<% unless version = 'ga' -%>...<% else -%>...<% end -%>`.


Suggested change

- If any of the added Go code (including any imports) is beta-only, change the file suffix to `.go.erb` and wrap the beta-only code in a version guard: `<% unless version = 'ga' -%>...<% else -%>...<% end -%>`.

- If any of the added Go code (including any imports) is beta-only, change the file suffix to `.go.erb` and wrap the beta-only code in a version guard: `<% unless version == 'ga' -%>...<% else -%>...<% end -%>`.

roaks3 · 2023-08-04T21:31:09Z

@melinath Nice catch on those!

modular-magician · 2023-08-04T21:41:47Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

modular-magician · 2023-08-04T23:00:24Z

Tests analytics

Total tests: 2931
Passed tests 2629
Skipped tests: 302
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

…#8307) * Merge test guides into a single task-based guide * Tweak test docs after initial review * Fix ref and replace use of standard tests * Update intructions for update tests to be more clear * Update resource doc to mention renaming needed for tests * Unindent warning * Fix version guards

roaks3 requested a review from melinath July 11, 2023 15:06

melinath requested changes Jul 11, 2023

View reviewed changes

roaks3 force-pushed the merge-test-guides branch from af4ce5c to 8e6cf1f Compare August 1, 2023 13:54

roaks3 added 3 commits August 3, 2023 10:51

Merge test guides into a single task-based guide

cd833a6

Tweak test docs after initial review

43af753

Fix ref and replace use of standard tests

222f23a

roaks3 force-pushed the merge-test-guides branch from 22c6e39 to 222f23a Compare August 3, 2023 16:00

roaks3 added 2 commits August 4, 2023 12:04

Update intructions for update tests to be more clear

ff6f9f4

Update resource doc to mention renaming needed for tests

87a7a70

roaks3 requested a review from melinath August 4, 2023 16:26

melinath approved these changes Aug 4, 2023

View reviewed changes

Unindent warning

811581c

melinath approved these changes Aug 4, 2023

View reviewed changes

Fix version guards

368befd

melinath approved these changes Aug 4, 2023

View reviewed changes

roaks3 merged commit 1680e15 into main Aug 4, 2023

roaks3 deleted the merge-test-guides branch August 4, 2023 23:01

	- Wrap the beta-only tests in a version guard: `<% unless version = 'ga' -%>...<% else -%>...<% end -%>`.
	- Wrap the beta-only tests in a version guard: `<% unless version == 'ga' -%>...<% else -%>...<% end -%>`.

	- If any of the added Go code (including any imports) is beta-only, change the file suffix to `.go.erb` and wrap the beta-only code in a version guard: `<% unless version = 'ga' -%>...<% else -%>...<% end -%>`.
	- If any of the added Go code (including any imports) is beta-only, change the file suffix to `.go.erb` and wrap the beta-only code in a version guard: `<% unless version == 'ga' -%>...<% else -%>...<% end -%>`.

Uh oh!

Conversation

roaks3 commented Jul 11, 2023

Uh oh!

modular-magician commented Jul 11, 2023

Diff report

Uh oh!

modular-magician commented Jul 11, 2023

Tests analytics

Action taken

Uh oh!

modular-magician commented Jul 11, 2023

Uh oh!

melinath left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

modular-magician commented Aug 1, 2023

Diff report

Uh oh!

modular-magician commented Aug 1, 2023

Diff report

Uh oh!

modular-magician commented Aug 1, 2023

Tests analytics

Action taken

Uh oh!

modular-magician commented Aug 1, 2023

Uh oh!

modular-magician commented Aug 4, 2023

Diff report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

modular-magician commented Aug 4, 2023

Tests analytics

Uh oh!

modular-magician commented Aug 4, 2023

Diff report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roaks3 commented Aug 4, 2023

Uh oh!

modular-magician commented Aug 4, 2023

Diff report

Uh oh!

modular-magician commented Aug 4, 2023