Skip to content
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

Add -template option for testify #109

Merged
merged 14 commits into from
Nov 9, 2019
Merged

Add -template option for testify #109

merged 14 commits into from
Nov 9, 2019

Conversation

shihanng
Copy link
Contributor

Close #47

Allow test codes using github.com/stretchr/testify/assert to be generated with the following option

gotests -w -template=testify ...

Hi, @cweill , this PR is based on the discussion on #47. Thank you for reviewing.

@googlebot googlebot added the cla: yes Contributor signed Google CLA label Oct 22, 2019
@shihanng shihanng marked this pull request as ready for review October 22, 2019 06:50
@coveralls
Copy link

coveralls commented Oct 22, 2019

Coverage Status

Coverage increased (+0.1%) to 96.226% when pulling de3c7fd on shihanng:testify into 17e4e80 on cweill:develop.

@shihanng
Copy link
Contributor Author

shihanng commented Oct 22, 2019

🤔 I don't think I can cover these bad paths without purposely introducing ill-formatted templates into the new templates directory...

https://coveralls.io/builds/26460602/source?filename=internal%2Frender%2Frender.go#L62

Copy link
Owner

@cweill cweill left a comment

Choose a reason for hiding this comment

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

Looks good to me, though I'm no expert in testify. Please see my comment below.

testdata/goldens/template_testify.go Show resolved Hide resolved
@shihanng
Copy link
Contributor Author

shihanng commented Oct 27, 2019

@cweill I've made changes on the function template so that it now supports generating test cases without using subtests and also supporting -i print inputs option. The name argument will be used for these cases.

Please have another look. Thank you.

@shihanng shihanng requested a review from cweill October 27, 2019 07:13
Copy link
Owner

@cweill cweill left a comment

Choose a reason for hiding this comment

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

@shihanng: This is looking good. Almost ready to be merged. Only thing is test coverage is dropping about 1%. Could you please add the following test cases:

  1. Pass an argument to --template that is invalid.
  2. Pass an argument to --template that is a directory, but the templates are unparseable/invalid.

With those tests, I feel good about merging this PR.

internal/render/render.go Outdated Show resolved Hide resolved
@shihanng
Copy link
Contributor Author

shihanng commented Nov 7, 2019

@cweill, thank you for the comments. Regarding the following:

  • Pass an argument to --template that is invalid.

I think this is covered in https://github.com/cweill/gotests/pull/109/files#diff-2831232d50f25c12514b6d6ceaaee101R675-R681?

  • Pass an argument to --template that is a directory, but the templates are unparseable/invalid.

Since we are embedding the testify template as binary assert using esc, it means that we need to embed "fake" template (can be named test) in order to create this test case. It's my understanding correct?

@cweill
Copy link
Owner

cweill commented Nov 8, 2019

I think this is covered in https://github.com/cweill/gotests/pull/109/files#diff-2831232d50f25c12514b6d6ceaaee101R675-R681?

Okay, looks good.

Since we are embedding the testify template as binary assert using esc, it means that we need to embed "fake" template (can be named test) in order to create this test case. It's my understanding correct?

That sounds good to me. Let me know once that's done, and I'll approve.

- Test with empty directory

- Test with invalid template (copied from testify and removed a
  bracket)
For testing invalid template, we only need one template that is
invalid and the others can be empty files.
@shihanng
Copy link
Contributor Author

shihanng commented Nov 9, 2019

@cweill thanks for the advice. I've added two test cases:

  • Test with empty directory
  • Test with invalid template

@shihanng shihanng requested a review from cweill November 9, 2019 13:44
Copy link
Owner

@cweill cweill left a comment

Choose a reason for hiding this comment

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

@shihanng LGTM! Thanks for this PR.

This is very good work. Thank you for your patience.

@cweill cweill merged commit 12fe222 into cweill:develop Nov 9, 2019
@shihanng shihanng deleted the testify branch November 10, 2019 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Contributor signed Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to optionally use testify
4 participants