Correctly apply offset to custom timestamp value (fixes #563)

[sveinse]

Fix issue when providing a custom timestamp. Updated the test case to test provided and not provided timestamp.

Fixes #563

[acolomb]

Do we really want to utilize the timestamp module itself to verify test results? I think the previous approach of verifying raw message bytes is more robust and explicit. You could still do the decoding and "semantic" verification after confirming that the correct data was received. But that's actually testing two parts of the module functionality, where currently you remove the actual check for the first.

[sveinse]

I added back the direct validation of the timestamp and compare it with the bus output. Its good a good thing to do, as it verifies the value of the date based tests as well.

The previous value in the test were wrong thou. The timestamp 1486236238 does not correspond to encoding of b"\xb0\xa4\x29\x04\x31\x43" as the current test suggest. This timestamp is in the unit of the canopen epoch (1984), not the UNIX timestamp epoch. The function documentation clearly states unix timestamp, so one of them must change. I changed the input number into a unix timestamp, keeping the encoded bus data the same.

[acolomb]

I'm beginning to wonder whether changing the docs would be the better way then. We have a test to catch if the behavior changes. But instead of keeping the behavior consistent, we now change the behavior and adjust the test. Not what I'd call API stability, which the test was meant to guarantee, no?

I'll have to ponder this a bit more, can't really dig into this right now.

[sveinse]

I humbly beg to differ. Using other epochs than unix epoch is rare and somewhat an oddity. It's ok that this is epoch of choice in canopen communication is their 1984 epoch, but outside that CAN transport, its unlikely that these values are used directly in the application code. We use the canopen library to avoid dealing with how the busdata is encoded. As a consumer of a communication API, I would frown if it used a datestamp in seconds since 2000 as an example. Unix timestamp is the defacto standard and I think #563 is legitimate.

[acolomb]

Good arguments 👍

[acolomb]

Okay I think I'm getting it now. You're doing three things to these unit tests:

1. Introduce another way to verify the CANopen epoch offset number.
   → This can very well live as its own test function, no need to intermingle with the timestamp producer service test.

2. Correcting the existing test vector to actually match the expected message data. Which originally indicated a date in 2031, not in 2017 as assumed. Documenting the expected outcome in human readable form is valuable here.

3. Adding a second test vector using the current system time during the tests. To get the same value for comparison inside and outside the library code, you're patching the time function. Fine, good idea to introduce some slight "fuzzing" approach. I don't expect any flakiness because of this. It exercises the code path where transmit() doesn't get an argument. Because there is no constant message data to compare against, you're calculating the inverse of what transmit() does, utilizing the library's own internal constants.

The rather complex calculations are only needed for the latter test vector, as that one is dynamic. We don't need it for the constant case (2), that can stay as is except for the correction. And it doesn't need to run within the context manager block.

I'm a bit skeptical about re-using internal module constants for the tests. IMHO they should test the external API surface, independently from the internal implementation specifics. So at least we can replace the ONE_DAY and TIME_OF_DAY_STRUCT uses with hard-coded values for the test code. The OFFSET parameter is a magic number I'd consider public API (although undocumented).

I'll push a commit where I've tried to untangle these three changes, please have a look whether that's clear to you. It certainly made things clearer for me ;-)

[sveinse]

With all due respect, aren't we putting at lot of scrutiny into this one test case now? We have more than enough to chew in the main project... Please feel free to make the adjustments you see fit.

[acolomb]

Sure, sorry for the noise. I just needed some thinking out loud I guess, to understand what's going on here. As unit tests are all about correctness, I wanted to understand what you're proposing and the original diff was just too much to see directly.