Pressure Monitoring Systems errors in golang version

[piratax007]

Hi everyone,

In the Go version of the pressure monitoring systems exist an error in the 25 line of the sensor.go file because pressureTelemetryValue always will be the same value and is because s (in line 23) takes the same seed all the time, then, the value that assume is the same all the time.

In another hand, this kata doesn't is useful like a mocks exercise and is because to test the check function of the alarm, all that is needed is to create an instant of the alarm with different threshold, in the same way, is possible to create an instance of the sensor using anonymous functions to change the offset and samplePressure values, in that way is not needed to make a mock of anything.

[emilybache]

Ok so we should remove the seed so the behaviour is actually genuine. The threshold values in teh Alarm are supposed to be fixed at compile time actually. So maybe this translation isn't perfect?

However I'm interested in why you don't need a mock for the Go version where you do for other language versions. Can you show me a solution that doesn't use mocks and doesn't change the Alarm class?

[piratax007]

Ok, thanks for your answer,

The thing is, yeah! someone can use mocks to test the alarm.check() but in a TDD approach the first thing to do is to understand what is doing the function that we want to test and always start with the simplest way, then, is the mock approach the simplest?. This is my code to test the check function without using a mock of the sensor:

func (s *kataSuite) Test_alarm_check_returnsFalseIfThePressureIsBetweenTheThreshold() {
    a := &alarm{
        14,
        24,
        NewSensor(),
        false,
    }

    a.check()

    s.False(a.alarmOn)
}

func (s *kataSuite) Test_alarm_check_returnsTrueIfThePressureIsOutFromTheThreshold() {
    testSensor := sensor{
        func() int {
            return 10
        },
        func() int {
            return 6
        },
    }

    a := &alarm{
        18,
        21,
        testSensor,
        false,
    }

    a.check()

    s.True(a.alarmOn)
}

[emilybache]

Thankyou so much for the code, that makes your query clearer for me.

So the first test shouldn't be possible because you shouldn't be able to change the thresholds to 14 and 24 at runtime. I don't know Go well enough to know how to achieve that in the starting code position. Can these fields in the struct somehow be declared final or private in a way so only the constructor/factory method can assign them?

The second example I would argue is in fact using a stub. It's a hand-coded anonymous class that you assign to the 'testSensor' variable. In the Java or C# version you wouldn't have been able to assign this field in Alarm without changing the alarm code but evidently you can assign it like this in Go. Is this another translation problem?

[piratax007]

Ok, although you can use constants to try blocking change in some values in the definition of the alarm structure, I think that a better way is to access to alarm and sensor through an interface like an API in order to do mandatory use mocks

The other problem is the rand.Intn(1) call. This will return [0,1), which means it will always return 0, because the 1 is the exclusive edge of the range. In Golang you will need to ask for a floating point number instead, using rand.Float32 or rand.Float64. Also, if you do not have a seed, the int/rand functions will all be seeded with 1 from the beginning of the program, meaning they will generate the same sequence. If you need actual randomness, you can use crypto/rand instead.

[emilybache]

ok so the random call is really supposed to come up with a random float. If it's not then that's definitely a translation mistake! I don't particularly mind if it's seeded the same every time, as long as it prevents you from writing tests that pass consistently without addressing the dependency.

[codecop]

If we access Alarm from outside its module tpms, we would not be able to use the struct directly (because it is lowercase, so not exported) but would need to use the "constructor" function NewAlarm. The sample test is using this constructor, but because it is in the same package, it can access the struct, as shown by piratax007. I believe there is no way to hide the struct completely.

[codecop]

Also the Sensor is not like in the other languages. It is parametrized, which makes it available to be used as a stub, it should just be a function of an interface probably.

[piratax007]

The Sensor and Alarm have different functionalities, to make a clear context, these must have their own package that exports only the functionality that is needed, so each one must be defined in separate packages.

Regarding the random seed, the problem is not with the test because if you use something like a mock, your test shadow the real random function to verify the behaviour of that function because you are not testing the rand.Intn function, the thing is that using this kind of randomness is completely wrong, so I mean, maybe in an academic exercise this randomness is useful nevertheless is a good idea to do a remake about that.

[emilybache]

Ok so sounds like the go version needs a re-write where Alarm and Sensor are in different packages so they can have a clearer interface. The randomness is there to make sure the Sensor is an awkward collaborator - you shouldn't be able to just use the real one when testing Alarm. That's the point. My skills with Go are rather limited - would either of you kind people like to make an attempt at improving the Go version and sending a pull request? Also - I have another repo where I'm putting new language versions so you could just go and send the pull request there straight away? https://github.com/emilybache/TirePressure-Kata

[codecop]

This issue still persists, it's a perfect fit for a first pull request for any aspiring Go developer ;-)