Skip to content

Conversation

@StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Nov 17, 2022

Our MSVC-internal "Contest" infrastructure uses some machines running older OSes, which don't understand ICU and UTF-8 locales. We had removed test workarounds in #2791 (2022-06-15), operating under the belief that this infrastructure would be updated soon. That hasn't happened yet. The affected tests are showing up as "flaky test failures" whenever any MSVC devs run the std or libcxx suites in Contest.

This PR contains the test workarounds that I've been temporarily applying whenever I mirror GitHub PRs to MSVC. (It's a bit more than just reverting the relevant changes in #2791, as we've had an LLVM update since then.) Note that the changes to tests/libcxx/skipped_tests.txt occur at the bottom, which is an MSVC-only section, and therefore are not replicated to expected_results.txt (which is for the GitHub/Python harness).

Fixes VSO-1601209.

@StephanTLavavej StephanTLavavej added the test Related to test code label Nov 17, 2022
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner November 17, 2022 00:21
} catch (const system_error& ex) {
if (ex.code() == error_code{126 /* ERROR_MOD_NOT_FOUND */, system_category()}) {
// Skip testing when we can't load icu.dll on an internal test machine running an older OS.
exit(EXIT_SUCCESS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should really be "unsupported" or something.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle, I agree. Our ancient <pmretvals.h> infrastructure had different exit codes for different reasons, like "skip" (closest in meaning to "unsupported"), and "cascade" versus "fail" (with "cascade" meaning "couldn't even run", usually due to compiler errors). In practice, although I believe the MSVC-internal harness still respects the old codes, I don't see what returning "unsupported"/"skip" would improve for us.

#if !defined(_DLL) || _ITERATOR_DEBUG_LEVEL == DEFAULT_IDL_SETTING
test_locale<wchar_t>();
test_locale<char>();
#ifndef _MSVC_INTERNAL_TESTING // TRANSITION, the Windows version on Contest VMs doesn't always understand ".UTF-8"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there some internal ticket for fixing this we can reference?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I've filed internal IcM-349538021 "Outdated OSes on private Contest runners" to track this.

We haven't cited IcM "incidents" in the source code before; given your approval I will assume that the TRANSITION comments, git/PR history, and the filed incident are sufficient for us to remember what happened here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd appreciate having https://portal.microsofticm.com/imp/v3/incidents/details/349538021/home appear in the commit message for traceability.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want it in the MSVC-PR only, or also the GH PR?

Copy link
Contributor

@CaseyCarter CaseyCarter Nov 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer both. (I want the IcM URL to show up in git blame for GitHub.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welp, now that the IcM ticket has been closed and Contest supposedly decommissioned, shall we abandon this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not totally decommissioned, as apparently some LKG test runs are still running Contest-Private checks, but I'd be happy to abandon this PR. We've finally officially Gotten The Memo about running runall-private.ps1, I've verified that it succeeds without workarounds for x86 and x64, and we've clearly survived for months with the status quo. (Reportedly, Phase 6 will truly put an end to Contest.)

@StephanTLavavej StephanTLavavej deleted the contest-workarounds branch November 18, 2022 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Related to test code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants