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

区分signature错误和ak错误 #99

Merged
merged 1 commit into from Jan 10, 2019

Conversation

ssy352959096
Copy link
Contributor

No description provided.

try {
client.distinguishError(request, error, baseResponse);
} catch (Exception e) {
Assert.assertEquals("500 : ServerException", e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

如果上面没有抛错呢?

Copy link
Contributor

Choose a reason for hiding this comment

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

如果期待报错,用ExpectedException来做

Copy link
Contributor

Choose a reason for hiding this comment

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

distinguishError()这个方法建议改成private的,不能为了方便测试而改成public的

Copy link
Contributor

Choose a reason for hiding this comment

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

DefaultAcsClient不太适合提供distinguishError的功能,并且暴露给用户,这个函数建议可以放到工具类里面,这样也容易测试

try {
client.distinguishError(request, error, baseResponse);
} catch (Exception e) {
Assert.assertEquals("InvalidAccessSecret : Specified Access Key Secret is not valid.", e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

review 的时候说过多少次了 InvalidAccessKeySecret 不是 InvalidAccessSecret

try {
client.distinguishError(request, error, baseResponse);
} catch (ClientException e) {
Assert.fail();
Copy link
Contributor

Choose a reason for hiding this comment

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

这里要断言,code 不是 InvalidAccessKeySecret

Copy link
Contributor

@JacksonTian JacksonTian left a comment

Choose a reason for hiding this comment

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

有2个小问题要修改下

Mockito.when(error.getErrorCode()).thenReturn("error");
try {
client.distinguishError(request, error, baseResponse);
} catch (ClientException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

同上。

}
}

public void distinguishError(AcsRequest request, AcsError error, HttpResponse baseResponse) throws ClientException {
Copy link
Contributor

Choose a reason for hiding this comment

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

public -> private 是不是好一点

}
}

public <T extends AcsResponse> T distinguishError(AcsRequest<T> request, AcsError error, HttpResponse baseResponse) throws ClientException {
Copy link
Contributor

Choose a reason for hiding this comment

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

说了一天了 这个 public 还在。

@codecov-io
Copy link

codecov-io commented Jan 10, 2019

Codecov Report

Merging #99 into master will increase coverage by 0.84%.
The diff coverage is 70%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #99      +/-   ##
============================================
+ Coverage     70.92%   71.77%   +0.84%     
- Complexity      845      856      +11     
============================================
  Files            88       88              
  Lines          3006     3015       +9     
  Branches        305      308       +3     
============================================
+ Hits           2132     2164      +32     
+ Misses          809      782      -27     
- Partials         65       69       +4
Impacted Files Coverage Δ Complexity Δ
...core/src/main/java/com/aliyuncs/RpcAcsRequest.java 86.31% <100%> (+0.14%) 20 <0> (ø) ⬇️
...dk-core/src/main/java/com/aliyuncs/AcsRequest.java 79.67% <100%> (-0.33%) 45 <0> (ø)
...e/src/main/java/com/aliyuncs/DefaultAcsClient.java 23.56% <64.7%> (+15.4%) 10 <0> (+8) ⬆️
...main/java/com/aliyuncs/profile/DefaultProfile.java 66.17% <0%> (+2.94%) 18% <0%> (+1%) ⬆️
...c/main/java/com/aliyuncs/reader/ReaderFactory.java 16.66% <0%> (+16.66%) 1% <0%> (+1%) ⬆️
...a/com/aliyuncs/auth/StaticCredentialsProvider.java 68.75% <0%> (+31.25%) 2% <0%> (+1%) ⬆️

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 e29f54c...9785f50. Read the comment docs.

@sjj3086786
Copy link
Contributor

+1

@ssy352959096 ssy352959096 deleted the AcsRequest branch January 16, 2019 10:32
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.

None yet

4 participants