Test data-structure sizes in 32-bit Windows on CI, and fix size_of_hasher
#1932
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since it is still running in a 64-bit environment, and running the whole test suite is slow on Windows, this will likely not be worth keeping in full. It may subsequently be modified to run only some restricted subset of the tests, so that it completes faster.
To see if it produces build failures or new test failures on CI.
- Use platform-default Windows toolchain with 32-bit target again. - Remove the "cargo check default features" step. - Filter the tests so only those with "size" in their name are run. - Rename job from to `test-32bit-windows-size`.
Due to ABI differences between different 32-bit targets, the `size_of_hasher` test wrongly failed on `i686-pc-windows-msvc`. Although the test case with that name was introduced in GitoxideLabs#1915, the failure is actually long-standing, in that an analogous faiure occurred in the old `size_of_sha1` test that preceded it and on which it is based. That failure only happened when the old `fast-sha1` feature was enabled, and not with the old `rustsha1` feature. It was not detected earlier as that target is irregularly tested, and built with `--no-default-features --features max-pure` more often than other targets due to difficulties building some other non-Rust dependencies on it (when not cross-compiling). Since GitoxideLabs#1915, the failure is easier to detect, since we now use only one SHA-1 implementation, `sha1-checked`, so the test always fails on `i686-pc-windows-msvc`. This is only a bug in the test itself, not in any of the code under test. This commit changes the test to use `gix_testtools::size_ok`, which makes a `==` comparison on 64-bit targets but a `<=` comparison on 32-bit targets where there tends to be more variation in data structures' sizes. This is similar to some of the size assertion fixes in GitoxideLabs#1687 (77c3c59, fc13fc3).
EliahKagan
changed the title
size_of_hashersize_of_hasher
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Due to ABI differences between different 32-bit targets, the
size_of_hashertest wrongly failed oni686-pc-windows-msvc.Although the test case with that name was introduced in #1915, the failure is actually long-standing, in that an analogous faiure occurred in the old
size_of_sha1test that preceded it and on which it is based. That failure only happened when the oldfast-sha1feature was enabled, and not with the oldrustsha1feature. It was not detected earlier as that target is irregularly tested, and built with--no-default-features --features max-puremore often than other targets due to difficulties building some other non-Rust dependencies on it (when not cross-compiling).Since #1915, the failure is easier to detect, since we now use only one SHA-1 implementation,
sha1-checked, so the test always fails oni686-pc-windows-msvc. This is only a bug in the test itself, not in any of the code under test.This pull request makes two changes:
Detect this, future such test bugs, and possible size-related regressions in tests as well as in the code under test on 32-bit Windows, by adding a
test-32bit-windows-sizeCI job that runs test cases across the workspace that havesizein their names on thei686-pc-windows-msvctarget.Fix
size_of_hasherby adjusting it to usegix_testtools::size_ok, which makes a==comparison on 64-bit targets but a<=comparison on 32-bit targets where there tends to be more variation in data structures' sizes. This is similar to some of the size assertion fixes in #1687 (77c3c59, fc13fc3).The new CI job validates the test fix, but I am not sure if we should ultimately keep the new CI job. When it is not able to gain anything from caching of Rust dependencies, it takes about 8 minutes. This is shorter than any of the other Windows test jobs, but it is only running a small handful of tests.
If caching does not speed it up, then it might make sense to remove it eventually, especially if and when further Windows test jobs conferring more substantial beneift are added (such as to test that fixtures and Git configuration interaction work in an environment with MinGit, with the Git for Windows SDK, or in native Windows containers). For now I think it may be okay to have it.