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

Hash-order independent tests #550

Closed
c-blake opened this issue Jul 1, 2024 · 5 comments
Closed

Hash-order independent tests #550

c-blake opened this issue Jul 1, 2024 · 5 comments

Comments

@c-blake
Copy link
Contributor

c-blake commented Jul 1, 2024

tests/testhttpserver.nim(850, 12) and (852) fail with a new string hasher because they assume a specific order.

Any chance you guys can do something like @Vindaar did here in the next few days? Really just add a .sorted and put the @[] in sorted order.

c-blake added a commit to c-blake/nim-chronos that referenced this issue Jul 2, 2024
hash order issue allowing nim-core to change its string hash function
without making this test fail.
@c-blake
Copy link
Contributor Author

c-blake commented Jul 2, 2024

Sorry - I guess the line number of the test moved from the last release to the current HEAD of this repo. It was actually the area indicated by this PR.

The question remains - when is the next release of chronos planned? This error is blocking the Nim CI which is blocking a Nim-2.2 release.

@c-blake
Copy link
Contributor Author

c-blake commented Jul 2, 2024

Also, tagging @arnetheduck since the release cadance of this repo is somewhat slow compared to my impression of what @Araq means by "imminent", since he was the last person to tag a release, and since he said it should be merged ASAP, but that was ambiguous between "merged under its current non-disruptive --define=nimPreviewHashFarm" or "merged as the new default with a define for the old murmur".

@arnetheduck
Copy link
Member

Sorry - I guess the line number of the test moved from the last release to the current HEAD of this repo. It was actually the area indicated by this PR.

Can you open a PR for this? The link leads to a commit in your repo.

@c-blake
Copy link
Contributor Author

c-blake commented Jul 4, 2024

Done. Sorry - I actually did try to do that at first and something wasn't working then. This is the PR cross-linked for convenience: #551

@c-blake
Copy link
Contributor Author

c-blake commented Jul 9, 2024

The mentioned PR now passes the CI and just needs to be merged. So, I am closing this issue.

@c-blake c-blake closed this as completed Jul 9, 2024
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

No branches or pull requests

2 participants