Property based Unit testing

[Geolykt]

Property based unit testing will allow us to spot bugs we wouldn't have found traditionally, granted, unit testing for positions and locations won't get us far, but hey!

I should note that the commit will increase the time it takes to complete unit tests by a LOT, more specifically by a few seconds, where as it previously took only a few milliseconds to complete. However, we only test a few thousands of states, where as good unit tests would need to test millions or more states to spot bugs efficently. It's also adviced to increase the number of assertions, as each assertion reduces the amount of possible wronng implementations by a lot. In essence however the longer compuations time is a trade-off that is worth it.

I should also note that I've been somewhat lenient on the distance calculations, the reasoning behind that are floating point errors

[0ddlyoko]

I haven't tested these changes but if it's work, it's ok

[Geolykt]

K, done

[0ddlyoko]

Could you rebase your branch on top of develop? It looks like you're one commit behind develop and this commit has edited Position.py class https://github.com/tazz4843/McPy/commit/483aeecf27ba8260b3c938dda8eb2a7a5d7dea99 (Just to be sure tests work with the develop branch)

[Geolykt]

Well, the tests should be universal (or, more specifically should test the implementation against the specification), so if they do not work on the develop branch, then it's the develop branch that is at fault. Not to do an unnessary rebase as the unit tests should not know the implementation

[0ddlyoko]

Well, if the tests should be universal, so if they do not work on the develop branch, then it's the develop branch that is at fault. Not to do an unnessary rebase

I know but the last commit has fixed a bug in a method that is used in your test The actual method (clone_rounded) in Position.py file is bugged

[Geolykt]

Do you get the point of unit testing? Unit tests should not value the implementation over the specification, but the specificatio nover the implementation. So if it's bugged and it detects it, it's a good sign (it won't detect it though as I've given up on doing it quite so preciese without in essence reimplementing the specification, which would be pointless). So if a implementation returns the value "hello" but the specification says it should return "world", then it'S the job of the unit tests to check that the implementation does not return "hello", but "world"

[0ddlyoko]

There is currently an issue as a rounded y should be (y % 1 = 0.5), but curently is (y % 1 = 0.0), but we'll see that later

This is the normal behaviour and not an issue, rounded x, y & z should be integers (like 10) and not floats (like 10.5)

[Geolykt]

Because that is current behaviour, but it isn't the behaviour that we want (or at least we should add to the docs of the method that it does not center, but actually rounds)

[0ddlyoko]

Because that is current behaviour, but it isn't the behaviour that we want (or at least we should add to the docs of the method that it does not center, but actually rounds)

The name of the method is clone_rounded, it should clone the Position (or Location) & round it. Should we add a message saying that the method round the location even if there is "round" in the name of the method ?

Anyway, tests are working but is a bit slow (like you said), is there a way to decrease the time it takes ? (it's not important because it's only for testing)

[Geolykt]

Optimising unit tests means that we could fail to see some certain use cases that would break the applicaiton, of course we could add a switch to all the unit tests to allow them to be toggled so we can only test a single feature instead of testing every feature, however I got worries that this could be abused

[0ddlyoko]

Okay np, I was thinking the same thing as you

[0ddlyoko]

So, can we merge it or is there any modification to do ?

[Geolykt]

I got nothing planned

[0ddlyoko]

Merged