milliseconds parsed incorrectly in `deserializeTime`

[davidboweninrupt]

Search terms you've used

I looked at all the existing issues.

Bug description

deserializeTime expects the millisecond part of the string to be exactly three characters.

Unfortunately, when it's not exactly three characters you get unexpected results. The function does not validate that the incoming string is three characters long.

To Reproduce

In the test case expectedTimeWithFractionalSeconds we supply as string with two characters that represents 420 milliseconds but the fixture checks the outcome is 42 milliseconds which is a mistake.

You see a similar problem if you use the example value of ".1337" too. In that case you get 1.337 seconds added to your time instead of 0.1337 seconds.

Minimal reproduction

Update the 42 in the fixture to be the correct 420.

Run npm test

Expected result

deserializeTime should produce the correct result with 420 milliseconds.

Actual result

deserializeTime produces the incorrect result with 42 milliseconds.

Environment

Use the unit tests for this project.

Additional information

This bug caused the xsd:dateTime sometimes displays slightly incorrectly issue over in Penny.

[davidboweninrupt]

I worked on a fix for this by having Time.millisecond being non-integer. However, that caused me to need to change serializeTime too.

I started to look around to enhance the tests but struggled because all the tests I was running were in the same unit so I had to keep fixing them one at a time as each run stopped on the first error.

As a result I ran out of time to work on this problem.

I was tempted to do a "quick fix" by having deserializeTime truncate the string to be the first three characters. That would stop the large-scale problems, but would still give incorrect results because it would always be the floor, not the rounded-off value when there were more than three digits. It would also not fix the problem when there were less than three digits.

Rather than submit a bad fix I decided to drop my code and create this issue instead.

[Vinnl]

Oh nice, that explains the root of the issue. In that case, I'd expect the fix to be something like this at https://github.com/inrupt/solid-client-js/blob/9314ead4bfe1d42c9587bfe6b88785c7ce47ecf7/src/datatypes.ts#L255-L257

  const utcMilliseconds = optionalMillisecondString
-    ? Number.parseInt(optionalMillisecondString, 10)
+    ? Number.parseInt(`0.${optionalMillisecondString}`, 10) * 1000
    : 0;