Skip to content

Using PowerMockito to mock System.getent;#2593

Merged
liuhaoyang merged 6 commits intoapache:masterfrom
flycash:EnvUtilsTest-fix
May 7, 2019
Merged

Using PowerMockito to mock System.getent;#2593
liuhaoyang merged 6 commits intoapache:masterfrom
flycash:EnvUtilsTest-fix

Conversation

@flycash
Copy link
Copy Markdown
Member

@flycash flycash commented May 5, 2019

Now, I read the document of PowerMockito and find the way to mock system class's static method.
So I remove the reflection code. I think it would be better to use PowerMockito and it can avoid the problem of compatability between difference OS and platform.

see EnvUtilsTest failed

  • Why submit this pull request?

  • Bug fix

  • New feature provided

  • Improve performance

  • Related issues


Bug fix

  • Bug description.

  • How to fix?


New feature or improvement

  • Describe the details and related test reports.

@wu-sheng wu-sheng requested a review from liuhaoyang May 5, 2019 15:28
@wu-sheng wu-sheng added this to the 6.2.0 milestone May 5, 2019
@wu-sheng wu-sheng added bug Something isn't working and you are sure it's a bug! test Test requirements about performance, feature or before release. labels May 5, 2019
@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented May 5, 2019

@liuhaoyang What do you think? Which one do you prefer?

@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented May 5, 2019

I think you should continue open appveyor to avoid this in next time, no matter which PR you think it is better.

@coveralls
Copy link
Copy Markdown

coveralls commented May 5, 2019

Coverage Status

Coverage decreased (-0.02%) to 16.395% when pulling 6e2f23b on flycash:EnvUtilsTest-fix into 4e71085 on apache:master.

@flycash
Copy link
Copy Markdown
Member Author

flycash commented May 6, 2019

It seems that the configuration of appveyor has some mistakes. In fact, I didn't find the config file appveyor.yml int root directory.
Do we need to add the configuration?

@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented May 6, 2019

@liuhaoyang is working on that, the PR is on the way.

@liuhaoyang liuhaoyang mentioned this pull request May 6, 2019
3 tasks
@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented May 6, 2019

@flycash The appveyor setting file has been merged. Could you open the test in your PR, and let's wait it pass.

@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented May 6, 2019

I have sync your branch, you should have it now.

Copy link
Copy Markdown
Member

@liuhaoyang liuhaoyang left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working and you are sure it's a bug! test Test requirements about performance, feature or before release.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants