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

Moved all GetEnv's calls to init step #445

Merged
merged 1 commit into from
May 28, 2019

Conversation

Zyqsempai
Copy link
Contributor

Issue #419

Description of changes: Moved all GetEnv's calls to init step

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@nckturner
Copy link
Contributor

nckturner commented May 3, 2019

Thanks for this cleanup! Can you just fix the unit test:

== RUN   TestGetWarmIPTargetState
--- FAIL: TestGetWarmIPTargetState (0.00s)
    ipamd_test.go:404: 
        	Error Trace:	ipamd_test.go:404
        	Error:      	Should be true
        	Test:       	TestGetWarmIPTargetState
    ipamd_test.go:405: 
        	Error Trace:	ipamd_test.go:405
        	Error:      	Not equal: 
        	            	expected: 5
        	            	actual  : 0
        	Test:       	TestGetWarmIPTargetState
    ipamd_test.go:414: 
        	Error Trace:	ipamd_test.go:414
        	Error:      	Should be true
        	Test:       	TestGetWarmIPTargetState
    ipamd_test.go:415: 
        	Error Trace:	ipamd_test.go:415
        	Error:      	Not equal: 
        	            	expected: 3
        	            	actual  : 0
        	Test:       	TestGetWarmIPTargetState
    ipamd_test.go:424: 
        	Error Trace:	ipamd_test.go:424
        	Error:      	Should be true
        	Test:       	TestGetWarmIPTargetState

@Zyqsempai
Copy link
Contributor Author

@nckturner Hey, I have some troubles with running tests locally, so I can't see why do they failing now. Can you help me with that?

@mogren
Copy link
Contributor

mogren commented May 7, 2019

@Zyqsempai Are you running on a Mac? If you have docker installed, make docker-unit-test should work.

@Zyqsempai
Copy link
Contributor Author

@mogren yes on MAC, thank you it helped, but I still have the same test report as in the Travis, not enought detailed, can i make it more verbose to get something similar to what @nckturner sent above?

@Zyqsempai Zyqsempai force-pushed the 419-init-env-vars branch 2 times, most recently from 95b84b6 to 937bf12 Compare May 21, 2019 12:45
@Zyqsempai
Copy link
Contributor Author

@nckturner @mogren PTAL

@nckturner
Copy link
Contributor

So it looks like you basically removed testIncreaseIPPool(), which I don't disagree with because I'm not a fan of most of the unit tests on this project. They basically test a whole bunch of implementation details and I think we'd be hard pressed to find much value in them. Considering we have some work going into integration testing right now, should we remove this test and consider refactoring and adding it back in a separate PR @mogren?

ipamd/ipamd.go Outdated Show resolved Hide resolved
ipamd/ipamd_test.go Show resolved Hide resolved
@Zyqsempai
Copy link
Contributor Author

@mogren PTAL

Copy link
Contributor

@mogren mogren left a comment

Choose a reason for hiding this comment

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

Sorry for all the back and forth, but this is a pretty important change. Thanks for fixing the issue, almost done!

ipamd/ipamd_test.go Outdated Show resolved Hide resolved
ipamd/ipamd.go Outdated Show resolved Hide resolved
ipamd/ipamd.go Outdated Show resolved Hide resolved
ipamd/ipamd_test.go Outdated Show resolved Hide resolved
@Zyqsempai
Copy link
Contributor Author

@mogren Done;) Thank you for your help!

Copy link
Contributor

@mogren mogren left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me!

@mogren mogren merged commit 73ba53b into aws:master May 28, 2019
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.

3 participants