search

cweill / gotests

Automatically generate Go test boilerplate from your source code.
Apache License 2.0
4.93k stars 346 forks source link

Add GOTESTS_TEMPLATE and GOTESTS_TEMPLATE_DIR environment variables #118

Closed shihanng closed 4 years ago

shihanng commented 4 years ago

Closes #116

coveralls commented 4 years ago

Coverage Status

Coverage remained the same at 96.226% when pulling 18bccc4eab5ed3dfe2de4b84ef56fe57c8c1eeda on shihanng:envvar into cf73e227c48415614a09fa5761ce65b92d2d7ba1 on cweill:develop.

shihanng commented 4 years ago

Hi @cweill , is there anything that I can do to improve this PR 🙏 ?

cweill commented 4 years ago

@shihanng Please add tests that checks that setting the environment has the intended behavior:

  1. Setting GOTESTS_TEMPLATE will use the template.
  2. Setting GOTESTS_TEMPLATE_DIR will use the dir.
  3. Setting GOTESTS_TEMPLATE and GOTESTS_TEMPLATE_DIR will use the template over the dir.
  4. Setting GOTESTS_TEMPLATE and the flag will prioritize the flag.
  5. Setting GOTESTS_TEMPLATE_DIR and the flag will prioritize the flag.
shihanng commented 4 years ago

@cweill, thanks for the review.

The first thing that came to my mind when trying to add test cases is to add them in /gotests_test.go. However, I found that it might difficult to do so as the reading from environment variables part is done before GenerateTests in /gotests/main.go.

I would like to suggest that we test the new valOrGetenv function in a new /gotests/main_test.go. This should cover the following two cases:

  1. Setting GOTESTS_TEMPLATE and the flag will prioritize the flag.
  2. Setting GOTESTS_TEMPLATE_DIR and the flag will prioritize the flag.

Then naturally

  1. Setting GOTESTS_TEMPLATE will use the template.

is already covered by existing test cases:

https://github.com/cweill/gotests/blob/62fe1b80bbe2c4c7cb0ab26d868795b47475ab90/gotests_test.go#L638-L645

  1. Setting GOTESTS_TEMPLATE_DIR will use the dir.
  2. Setting GOTESTS_TEMPLATE and GOTESTS_TEMPLATE_DIR will use the template over the dir.

are covered by

https://github.com/cweill/gotests/blob/62fe1b80bbe2c4c7cb0ab26d868795b47475ab90/gotests_test.go#L683-L691

What do you think about this proposal?

Note: According to the help message and test case I think 3. should be reversed "will use the dir over the template."

cweill commented 4 years ago

@shihanng: This proposal sounds good to me. If you make those changes, I will approve the PR. Thank you.

shihanng commented 4 years ago

@cweill , I've added the tests in 18bccc4. Thanks!