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

Need add custom assert function to TestCase. #18

Merged
merged 10 commits into from
Feb 13, 2018
Merged

Need add custom assert function to TestCase. #18

merged 10 commits into from
Feb 13, 2018

Conversation

qwerty2501
Copy link

@qwerty2501 qwerty2501 commented Jan 19, 2018

This pull request related with issue #17

Caution

This pull request has break of compatibility.

PR Contents

  • added member AssertFunc to TestCase.
  • implemented validation with custom assert func.
  • added some test cases.

Compatibility

This pull request has break of compatibility.
The user should modify TestCase initialization when the user using field nameless struct initializer.

[]TestCase{
    {"token", "123456", "Test token"},        //Before
    {"token", "123456", "Test token", nil},  //After
})

@gong023
Copy link
Contributor

gong023 commented Jan 19, 2018

I’m so sorry. I didn’t think this breaks compatibility yesterday. At that time, I thought we can keep compatibility if the value is interface somehow... it’s completely wrong. this is my fault. I should have checked more carefully…

Personally it’s difficult to accept this. One of the reasons is that we have to change any existing code written by httpdoc.
Furthermore, even if we fix all existing tests, I think it's annoying for users to pass nil as an assertion always. in the most cases, this assertion should be nil.

@tcnksm I want to know your opinion too. How do you think?
And mac os tests in travis don't work somehow. Do you have any idea?

@qwerty2501
Copy link
Author

I know and don't mind.
I noticed this modification breaks compatibility it when I implemented it.
But I posted PR to pose problem.
As a way to imagine immediately, I guess implement constructo function for httpdoc.TestCase to solve.
This is my opinion.

@gong023
Copy link
Contributor

gong023 commented Jan 19, 2018

do you mean something like this?

// construct
func NewTestCase(target string, expected interface{}, desc string) TestCase {
	return TestCase{ ... AssertFunc:  nil}
}

// testcode
validator.ResponseHeaders(t, []TestCase{
	NewTestCase("X-API-Version", "1.1.2", ""), // use NewTestCase usually
	{"Target", "Expected", "desc", func(...)}, // user can pass TestCase manually
}

@qwerty2501
Copy link
Author

Yes.
But It seems breaks comptibility too.
I'm sorry my idea is not very good.
We need to discuss for a while.
How about number of go-httpdoc user?
Do you want to must protect compatibility?

@gong023
Copy link
Contributor

gong023 commented Jan 23, 2018

@qwerty2501 Hello, sorry to have kept you waiting. I chatted with @tcnksm and @zchee in person, then we concluded that we can accept this if you add NewTestCase as you said.
We will tag the current latest commit, and tag new one after this is merged. So you don't have to worry about compatibility.
Can you work on it? If you are busy, I can send a PR to your repo.

@qwerty2501
Copy link
Author

I can work.
If it is early, tomorrow, even if it is late, this week.
If you hurry, please send PR.

@qwerty2501
Copy link
Author

I done.
Please confirm.

@zchee
Copy link
Contributor

zchee commented Jan 24, 2018

@qwerty2501 I’ll review it.
/cc @gong023 @tcnksm

Copy link
Contributor

@zchee zchee left a comment

Choose a reason for hiding this comment

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

@qwerty2501 Sorry for late reply. TravisCI was failed but it seems not related this change. We should fix any CI settings. cc: @tcnksm
This pull request seems to LGTM, approve.

@zchee
Copy link
Contributor

zchee commented Feb 2, 2018

@qwerty2501 I restarted TravisCI. I’ll merge it when passes CI.

@zchee
Copy link
Contributor

zchee commented Feb 2, 2018

@qwerty2501 Hmm, The CI test falls randomly :( We considering replacing CircleCI instead of TravisCI.
Anyway, I will continue to re-run until the passes CI test. Sorry to have kept you waiting.

@zchee
Copy link
Contributor

zchee commented Feb 2, 2018

@qwerty2501 @gong023 @tcnksm Maybe I found why TravisCI always failed.
The cause is that no-backward compatibility of gettimeofday on macOS Sierra with go1.6.4.
golang/go#16570
The .travis.yml config just osx, which will be uses 16.7.0(Sierra).

I will disable or fix osx with go1.6.4 build config.

@zchee
Copy link
Contributor

zchee commented Feb 9, 2018

@qwerty2501 Sorry for the late reply...
We stopped support Go 1.6 or previous on this pull request.
#20

Maybe your pull request will pass CI test. Could you rebase master?
Sorry for bothering you. Thanks.

@codecov-io
Copy link

codecov-io commented Feb 9, 2018

Codecov Report

Merging #18 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
+ Coverage    97.2%   97.28%   +0.07%     
==========================================
  Files           3        3              
  Lines         215      221       +6     
==========================================
+ Hits          209      215       +6     
  Misses          4        4              
  Partials        2        2
Impacted Files Coverage Δ
validate.go 97.46% <100%> (+0.2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea87361...71b49e6. Read the comment docs.

@zchee
Copy link
Contributor

zchee commented Feb 9, 2018

@tcnksm passes CI. Is it ready to merge?

@tcnksm tcnksm merged commit c06c8a8 into mercari:master Feb 13, 2018
@qwerty2501 qwerty2501 deleted the feature/custom-assert-func branch February 14, 2018 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants