RFC: Simplify version handling of UI tests.

[adamreichold]

Due to checking in error outputs these tests only work reliably on stable and not on intermediate version between our MSRV (currently 1.48) and the current stable version.

Hence this simplifies things to run only MSRV-compatible tests for the MSRV builds, anything else for stable builds except for those tests which require the nightly feature, i.e. the Ungil being distinct from the Send trait.

Finally, not_send3 is disabled when using the nightly feature since while Rc is not send, it also not GIL-bound and hence can be passed into allow_threads as it does not actually spawn a new thread.

[adamreichold]

bors try

[bors]

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• conclusion

[adamreichold]

bors try

[bors]

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• conclusion

[davidhewitt]

Due to checking in error outputs these tests only work reliably on stable and not on intermediate version between our MSRV (currently 1.48) and the current stable version.

Can you clarify where the current tests broke? It was a best effort but I was hoping that by gating tests on each version in the way it was currently done it should be the case that when a test output changed it would be bumped to run only on that rust version or newer. The effect hopefully being that older intermediate versions would only run tests which that version produced the same output as current stable.

[adamreichold]

Can you clarify where the current tests broke?

I ran the tests using 1.63 and immediately, meaning on main unmodified, got changes in the error output of 8 test cases.

It was a best effort but I was hoping that by gating tests on each version in the way it was currently done it should be the case that when a test output changed it would be bumped to run only on that rust version or newer.

I suspect we failed to do this at some point in the past when we updated the expected error outputs to match a new stable release. These test cases would have had to be moved to a later rustversion::since-gated section since their output retroactively changed from stable to stable, but we did not do that and hence they are broken when using an older compiler.

Personally, I don't think the effort to keep this aligned is worth it: We do not test these intermediate versions in the CI. (Which is also why we did not notice them breaking.) Even for MSRV and nightly, it seems enough to check that these test cases fail to build as expected. The detailed error message could then just be kept in sync with the current stable release and that would be it. Hence the change proposed here.

(With this change, we could also drop the dev dependency on rustversion after the MSRV bump to 1.56 because the only other use (besides xtask) is in the tests that do not even parse on 1.48.)

[davidhewitt]

Ah right. That makes sense and I'm in favour of simplification.

So the one concern I have is for system package distributors, who I think may sometimes run cargo test as part of their package build / distribution? If that's the case I suspect they use the distro's rust, which probably isn't latest stable.

That said, as you note, it's already broken. I suppose that if we go this route and only support the UI tests on latest stable, we can perhaps just recommend any distro packagers who hit this just ignore the UI tests.

[adamreichold]

So the one concern I have is for system package distributors, who I think may sometimes run cargo test as part of their package build / distribution? If that's the case I suspect they use the distro's rust, which probably isn't latest stable.

One idea I see to fix this is to just not ship tests/test_compile_error.rs on crates.io, i.e. adding it to package.exclude via Cargo.toml so that package distributors using the sources from crates.io do not get this particular test case at all. (We could also exclude tests/ui then to save a few bytes.)

While this may appear a bit heavy-handed, alternatives like environment variables or Cargo features seem to loose out on discoverability and brittleness.

[davidhewitt]

One idea I see to fix this is to just not ship tests/test_compile_error.rs on crates.io, i.e. adding it to package.exclude via Cargo.toml so that package distributors using the sources from crates.io do not get this particular test case at all. (We could also exclude tests/ui then to save a few bytes.)

That sounds like a very reasonable idea, as long as we don't have other references to those files which might otherwise break the distributed crate.

I suppose some vendors might be using the GitHub archive, they can always strip or ignore the UI tests themselves.

[adamreichold]

I excluded the UI tests from the package for crates.io and converted the nightly-related UI tests into doc tests which is actually nice since it gives a bit more context. Please have another look.

[davidhewitt]

A really nice improvement! Just one comment motivated by our new docs, which we might want to spin off into a separate discussion...

[davidhewitt]

Looks great, sorry for the delayed approval!

bors r+

[bors]

Merge conflict.

[adamreichold]

bors retry

[bors]

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• conclusion