Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tests to be string hash order independent #551

Merged
merged 9 commits into from
Jul 9, 2024

Conversation

c-blake
Copy link
Contributor

@c-blake c-blake commented Jul 4, 2024

For new Farm Hash-based hash(x:string).

c-blake and others added 2 commits July 2, 2024 04:45
hash order issue allowing nim-core to change its string hash function
without making this test fail.
@narimiran
Copy link
Contributor

I haven't explored the reported error in detail, but it looks like it might be a missing import std/algorithm problem, to have sorted on seqs.

@c-blake
Copy link
Contributor Author

c-blake commented Jul 8, 2024

std/algorithm is there right at the top of the file. I think it is more missing a < as the error message says. I pushed a commit to define one. Can you re-run the tests? Thanks.

not allowing definition of a comparison before such tests.
@c-blake
Copy link
Contributor Author

c-blake commented Jul 8, 2024

Another compile error, another git push to try to fix it. :-)

Needless to say, I am not running these tests locally (I am not set up to). Much appreciated if you could approve the workflow again.

like a compiler bug to report if it isn't already { but that's also a
behavior for a more isolated test just sorting one of the `seq` }.
@c-blake
Copy link
Contributor Author

c-blake commented Jul 8, 2024

Ok. Naming fields with tuple[] is non-optional. Dumb commit message.

This time I at least confirmed compilation & functioning locally of

import std/algorithm
proc `<`(a, b: (string,string)): bool = a[0] < b[0]
echo @[("Header2", "value2"), ("Header2", "VALUE3"), ("Header1", "value1")].sorted

So, I have high hopes this version will work. Thanks for approving the workflow yet again!

@c-blake
Copy link
Contributor Author

c-blake commented Jul 8, 2024

Ack. I just kept mis-reading compiler errors. This works for me locally and should work now if you approve the workflow yet again (famous last words, as they say!):

import std/[algorithm, assertions]
proc `<`(a, b: (string, seq[string])): bool = a[0] < b[0]
let data1 = @[("Header2", "value2"), ("Header2", "VALUE3"), ("Header1", "value1")]
let data2 = @[("Header2", @["value2", "VALUE3"]), ("Header1", @["value1"])]
assert data1.sorted == sorted(@[("Header2", "value2"), ("Header2", "VALUE3"),
                                ("Header1", "value1")])
assert data2.sorted == sorted(@[("Header2", @["value2", "VALUE3"]),
                                ("Header1", @["value1"])])

@narimiran
Copy link
Contributor

Heh, we're back at Error: type mismatch: got <seq[string], seq[string]>

@c-blake
Copy link
Contributor Author

c-blake commented Jul 8, 2024

I don't know what's wrong now. That above snippet really does run fine locally on nim-devel. I could add a <(a,b: seq[string]), but I'm not sure if that's ok. Do you know what's wrong? Is this some known gotcha with the test suite thing? Should I lift that < to outermost scope?

c-blake added 3 commits July 8, 2024 13:39
ringabout added a commit to nim-lang/Nim that referenced this pull request Jul 9, 2024
ref status-im/nim-chronos#551 which is in an uncertain state
@c-blake
Copy link
Contributor Author

c-blake commented Jul 9, 2024

Well, with yesterday's final commit (lifting < to outermost scope as suggested in the last comment here), the checks pass now on nim-devel (& older).

There may be other considerations in tagging a new Chronos release so as to pass the main Nim CI, but as far as this test failure is concerned, I think the work is done and this PR is ok to merge. Let me know if this is not the case.

Thanks

@narimiran narimiran merged commit 8f609b6 into status-im:master Jul 9, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants