Closed ergo720 closed 7 months ago
Is a fix PR on the CxbxReloaded kernel considered as well?
Looks good to me
Please do not merge this PR. There's an extended tests been implemented except is not migrate into this pull request. I'm currently awaiting on ergo720's inspection and decision on two suggestions.
https://github.com/Cxbx-Reloaded/Cxbx-Reloaded/pull/2441 has been merged. Can someone confirm this test now succeeds with a build of that?
@PatrickvL, I won't check because of pull request #90 is absolutely critical bugfix which isn't merged yet. Otherwise, this test is at risk of causing emulator/homebrew kernels/etc to hang if any failure is discover from this test or any tests that is over 4 bytes long variables.
Now that #90 is merged, attaching a positive result from a build of this test could be used as proof this can be merged as well.
Now that #90 is merged, attaching a positive result from a build of this test could be used as proof this can be merged as well.
The answer had been provided about 3 posts above. That is for the hardware testing.
Cxbx-Reloaded builds (before and after https://github.com/Cxbx-Reloaded/Cxbx-Reloaded/pull/2441 pull request) gave confusing same result. I'm not sure why yet.
EDIT: Turns out the bugfix changes is the same method.
Kernel Test Suite
build: a5d80e1
-----------------------------------------------------
0x0130 - RtlTimeFieldsToTime: Tests Starting
ERROR(line 112): Expected array time_tests[18].time_expected.QuadPart = 0x0, Got time_tests[18].time_actual.QuadPart = 0x26bffb7ecc000
ERROR(line 113): Expected array time_tests[18].result_expected = 0x0, Got time_tests[18].result_actual = 0x1
0x0130 - RtlTimeFieldsToTime: One or more tests FAILED
0x0131 - RtlTimeToTimeFields: Tests Starting
ERROR(line 159): Expected array time_fields_tests[12].expected.Year = 0x28a, Got time_fields_tests[12].actual.Year = 0xffff9416
ERROR(line 160): Expected array time_fields_tests[12].expected.Month = 0x5, Got time_fields_tests[12].actual.Month = 0xfffffff9
ERROR(line 161): Expected array time_fields_tests[12].expected.Day = 0xa, Got time_fields_tests[12].actual.Day = 0xfffffff7
ERROR(line 162): Expected array time_fields_tests[12].expected.Hour = 0x4a6, Got time_fields_tests[12].actual.Hour = 0xfffffffe
ERROR(line 163): Expected array time_fields_tests[12].expected.Minute = 0xe, Got time_fields_tests[12].actual.Minute = 0xffffffd0
ERROR(line 164): Expected array time_fields_tests[12].expected.Second = 0x29, Got time_fields_tests[12].actual.Second = 0xfffffffb
ERROR(line 165): Expected array time_fields_tests[12].expected.Millisecond = 0x333, Got time_fields_tests[12].actual.Millisecond = 0xfffffe23
0x0131 - RtlTimeToTimeFields: One or more tests FAILED
------------------------ End of Tests -----------------------
According to time_tests[18]
test (RtlTimeFieldsToTime), it is checking non-leap year's day that is expecting to be non-existence. However, Cxbx-Reloaded says it exists (which is wrong).
Then RtlTimeToTimeFields test for time_fields_tests[12]
which is overflow max large integer test. The overflow max large integer was a suggestion from me to see if we should use it (abuse the API) or drop the test.
The answer had been provided about 3 posts above. That is for the hardware testing.
Oh sorry, I should've looked better. Thanks.
Also good that you added an analysis of the remaining issues in CxbxReloaded. It's obvious I shouldn't have merged Cxbx-Reloaded/Cxbx-Reloaded#2441 ...
I forgot to include the weekday variable test for RtlTimeToTimeFields test.
I ran the test on hardware and it passed:
Kernel Test Suite
build: 6d76b16
Random seed used is 0
Config File Was Loaded. Only running requested tests.
-----------------------------------------------------
0x0130 - RtlTimeFieldsToTime: Tests Starting
0x0130 - RtlTimeFieldsToTime: All tests PASSED
0x0131 - RtlTimeToTimeFields: Tests Starting
0x0131 - RtlTimeToTimeFields: All tests PASSED
------------------------ End of Tests -----------------------
I noticed this in Cxbx-reloaded, where it seems that the implementation of this function is wrong. I believe that the function is supposed to return the time converted in 100 ns intervals, while now it seems it's only returning the ms elapsed from the Windows epoch time (January 1st, 1601). So I added these tests to confirm my suspicion. It's a draft for now because I haven't tested this yet on actual hardware. EDIT: confirmed as working on hardware