fix panic about float in storage port, and add a test that verifies it #501
Conversation
| c.Assert(err, gc.IsNil) | ||
|
|
||
| // really, we're asserting that this doesn't panic :) | ||
| c.Assert(env.(*manualEnviron).cfg.storagePort(), gc.Equals, int(8040)) |
There was a problem hiding this comment.
You could also use the gc.Panic/gc.PanicMatches checkers to test that it does panic when expected.
There was a problem hiding this comment.
Is the int cast on int(8040) necessary?
There was a problem hiding this comment.
As far as I understand it, at the point gocheck compares the values go has already inferred a type from the literal: int. So the cast is necessary.
There was a problem hiding this comment.
It really shouldn't ever be set to something that's not a float or an int.... the panic is basically just there in the cast because something has to happen there, and we can't return anything.
The cast to int is not necessary... the constant 8040 will default to becoming an int, but it does sort of make it a little more clear that the constant isn't getting implicitly cast to some other type (like float64).
|
LGTM |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Does not match ['fixes-1347715', 'fixes-1355324'] |
|
weird, |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
fix panic about float in storage port, and add a test that verifies it So, this __fixes-1347715__ - at least the most problem thing to crop up. We have tests in the code that were checking to make sure that if a float gets into the config via json deserialization, that we handle it correctly. However, it wasn't doing precisely the same thing the production code does, so it wasn't actually getting the value into the right area of the environConfig, and wasn't actually calling storagePort(). The test I added does this, and I verified it fails with a panic with the old code for manualConfig... then fixed the code the do a type assertion first, handling float64 and int... reran the test and now the test passes. My only concern is that I don't know what changed to cause this error to suddenly start occurring. I'm going to look into it and see if I can find the change that revealed this error.
|
We should be using the |
So, this fixes-1347715 - at least the most problem thing to crop up. We have tests in the code that were checking to make sure that if a float gets into the config via json deserialization, that we handle it correctly. However, it wasn't doing precisely the same thing the production code does, so it wasn't actually getting the value into the right area of the environConfig, and wasn't actually calling storagePort(). The test I added does this, and I verified it fails with a panic with the old code for manualConfig... then fixed the code the do a type assertion first, handling float64 and int... reran the test and now the test passes.
My only concern is that I don't know what changed to cause this error to suddenly start occurring. I'm going to look into it and see if I can find the change that revealed this error.