Skip to content
Snippets Groups Projects

Test coverage and refactoring in preparation for steamrt/tasks#595

Merged Simon McVittie requested to merge wip/task595-prep into main

Hopefully all of this is uncontroversial. Anything that attracts review comments should probably be split out into its own MR.

  • utils: Factor out _srt_string_ends_with_len()

    If the length is already known, this avoids recalculating it (and if the suffix contains embedded NULs, this makes it possible to check for).

  • tests: Add some missing coverage

    We never explicitly tested the case where _srt_string_ends_with() is looking for a suffix longer than the string itself.

  • tests: Add missing test coverage for what happens without PvPerArchDirs

    If we are unable to discover $LIB and $PLATFORM, or if they do not fit any of the patterns we expect, then we might have to skip reorganising the LD_PRELOAD and LD_AUDIT modules. Test this, to make sure refactoring does not make it regress.

  • tests: Ensure we have better coverage if built with --werror

    Release builds are not done with --werror, so we will gracefully skip various parts of the build if we are running on a particularly strange platform (where /dev/pts isn't a directory, /dev/stderr isn't a symlink, or we are unable to expand ${LIB} and ${PLATFORM}). But for our official CI and individual developers' builds, we expect the build to be done on a platform that makes sense, so we can be more strict.

  • tests: When building with --werror, make assertions about /etc/os-release

    We can expect and assume that on our official CI or on developer machines, we'll be running in a reasonable environment.

  • tests: Future-proof PvAdverbPreloadModule initializers

    This will let the test continue to pass without code changes when we add new struct members, as long as zero/NULL is a valid default value for those members, reducing the diffstat for refactoring in this area.

  • pv-wrap: Remove redundant reinitialization of SrtSystemInfo

    Instantiating this can't fail, and if it somehow does, trying it again is unlikely to help.

  • build-aux: Add convenience files to cross-compile for arm64

    This is a useful way to check that non-x86 builds aren't regressing, without actually needing a non-x86 device.

    Not all tests will pass in this configuration because qemu-user is not a perfect emulation of a real aarch64 machine (for example PR_GET_CHILD_SUBREAPER is unimplemented), but it's better than nothing. In particular, the adverb-preload and wrap-setup tests pass, and those are the ones I'm altering for steamrt/tasks#595.

  • pressure-vessel: Avoid looping through zero-length array of emulator hosts

    If compiled for a non-x86 architecture, we don't expect to be emulated by some foreign architecture with a mechanism like FEX or qemu-user, so PV_N_SUPPORTED_ARCHITECTURES_AS_EMULATOR_HOST is zero. In this case don't try to allocate and loop over an array of length 0. This also avoids gcc warnings with -Wtype-limits, "comparison of unsigned expression in ‘< 0’ is always false".

  • tests: Combine expected test results for x86_64 and non-x86

    There are three differences between our handling of x86 and non-x86 here:

    1. On x86, x86_64 is always treated as the primary architecture, with i386 as secondary (even if pressure-vessel itself is compiled as an i386 binary). On other platforms, the native platform is primary.
    2. On x86, i386 is available as a secondary architecture. On other platforms, there its no secondary architecture.
    3. We have different mock expansions for ${LIB}, ${PLATFORM} and the multiarch tuple on x86 and non-x86.

    By abstracting the "primary architecture", we can avoid having lines of test code that are only run on non-x86 platforms, which in practice means they are not routinely run at all. This makes them less likely to regress.

  • pv-wrap: Document PV_APPEND_PRELOAD_FLAGS_IN_UNIT_TESTS

    Fixes: 08751fff "tests: Assert that modules with ${LIB} or ${PLATFORM} are handled"

  • pv-wrap: Have test coverage for the single-architecture case

    Until now, the case where only one architecture is supported has only been tested if pressure-vessel was built for a non-x86 architecture such as ARM, which is rarely (if ever) done.

    We can get coverage for this case by making pv-wrap behave as if only the first architecture was supported, as a runtime rather than compile-time decision: this means that an ordinary x86_64 build can exercise both the x86 and non-x86 code paths.

    This is important because steamrt/tasks#595 will add new code that differs according to whether there is only a single architecture, or more than one.

    In the test, this means we need to be able to mark which of the expected paths are expected on i386 only, and which ones are expected in general. Instead of the verbosity that would result from turning all of our test data into structs, I've done this with an ad-hoc mini-language: the ones that are only expected on i386 are prefixed with i386:.

    We also might conceivably want to make use of something similar to PV_APPEND_PRELOAD_FLAGS_ONE_ARCHITECTURE in production in future, if we start providing legacy-free containers that only support running x86_64 games.

/cc @refi64 @denittis

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
Please register or sign in to reply
Loading