Skip to content

Conversation

@oklahomer
Copy link
Contributor

Fixes #179

To switch the API domain name, this p-r introduces a mechanism to explicitly set and pass base URL in each XxxCall implementation.
I noticed examples/richmenu_helper/main.go was missing "download" mode even though it was part of the acceptable mode list. To test integration 163f81f adds an implementation for "download."

@CLAassistant
Copy link

CLAassistant commented Nov 17, 2019

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

codecov-io commented Nov 17, 2019

Codecov Report

Merging #181 into master will increase coverage by 1.35%.
The diff coverage is 89.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #181      +/-   ##
==========================================
+ Coverage   74.19%   75.55%   +1.35%     
==========================================
  Files          25       25              
  Lines        1984     1996      +12     
==========================================
+ Hits         1472     1508      +36     
+ Misses        426      394      -32     
- Partials       86       94       +8
Impacted Files Coverage Δ
linebot/get_content.go 100% <100%> (ø) ⬆️
linebot/liff.go 68.42% <100%> (ø) ⬆️
linebot/get_ids.go 81.25% <100%> (ø) ⬆️
linebot/get_quota.go 100% <100%> (ø) ⬆️
linebot/insight.go 100% <100%> (ø) ⬆️
linebot/get_profile.go 100% <100%> (ø) ⬆️
linebot/richmenu.go 58.7% <100%> (+11.33%) ⬆️
linebot/delivery.go 69.69% <100%> (ø) ⬆️
linebot/client.go 72.09% <80%> (-0.88%) ⬇️

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 42fa460...3c28b82. Read the comment docs.

@oklahomer oklahomer force-pushed the feature/179-domain-name-change branch from f81b047 to 19b9b7c Compare December 10, 2019 01:13
@oklahomer oklahomer requested a review from nasa9084 December 10, 2019 07:47
dataServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
defer r.Body.Close()
w.WriteHeader(404)
w.Write([]byte(`{"message":"Not found"}`))
Copy link
Member

Choose a reason for hiding this comment

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

How about calling t.Error in this handler?
we want to know dataServer is not called

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3c28b82 applies changes to make the intention clearer.

Copy link
Member

Choose a reason for hiding this comment

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

should be responses 200? double error will be occurred

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m not sure where this p-r is going.

Just to make sure, are you saying that this should return the HTTP status code of 200 and the expected response? And only leave error state for the wrong API endpoint call? The tests I added kind of focus on the behavior of target API. It is somewhat like “If a wrong endpoint is called, then an unexpected response is returned and hence following test items fail.”

Honestly, the more I read tests in this repository I get more confused. For example, subtests are executed in a loop, but a httptest.Server is constructed outside of the loop and it still refers to testing.T which does not belong to the subtest. So the nested output of the test would look pretty weird.
And some request methods have benchmark tests but some do not. The benchmark actually points to a mock server so the main differences would be the time elapsed for payload serialization and deserialization. Probably this is because all XxxCalls currently have Do method so it seemed like all request methods needed to have benchmark tests as long as one was created for a certain method, but I am not sure about the consequence.

I started with a minimal code change before the first code review because a) this had to be merged prior to the notified date and b) some pull requests with potential conflicts were under review. But now it involves changes for overall architecture including the signature changes of mockClient while the deadline is getting close.

Probably I should close this p-r and let someone familiar with this repository’s policy take over so this issue can be handled in a short time.

@oklahomer oklahomer requested a review from nasa9084 December 22, 2019 22:43
@nasa9084 nasa9084 merged commit c78ca9b into line:master Jan 10, 2020
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.

Domain name change from api.line.me to api-data.line.me

4 participants