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.

Sign up for GitHub

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

Jump to bottom

Partially analyze some SKIPPED libcxx tests #4270

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Partially analyze some SKIPPED libcxx tests #4270

merged 1 commit into from
Jan 9, 2024

Conversation

cpplearner
Copy link
Contributor

No description provided.

@cpplearner cpplearner requested a review from a team as a code owner December 16, 2023 10:12
@StephanTLavavej StephanTLavavej added the test Related to test code label Dec 16, 2023
StephanTLavavej
StephanTLavavej approved these changes Dec 16, 2023
View reviewed changes
tests/libcxx/expected_results.txt
@@ -53,7 +53,8 @@ std/utilities/memory/default.allocator/allocator.ctor.pass.cpp FAIL
# LWG-3197 "std::prev should not require BidirectionalIterator" (New)
std/iterators/iterator.primitives/iterator.operations/prev.pass.cpp FAIL

# Itanium ABI assumptions that current_exception and rethrow_exception don't copy the exception object
# Itanium ABI assumptions that current_exception and rethrow_exception don't copy the exception object.
# The SKIPPED tests contain `XFAIL: msvc`, which is not well understood by the test harness.
Copy link
Member

Choose a reason for hiding this comment

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

No change requested: I would have said "the MSVC-internal test harness" here because the GitHub test harness does understand XFAIL: msvc, but it's not worth resetting testing, this comment is a strict improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests successfully compile but fail to run. So they are affected by the ARM and ARM64 issue that you explained in

STL/tests/libcxx/expected_results.txt

Lines 1346 to 1355 in a888880

# *** XFAILs WHICH PASS ***
# These tests contain `// XFAIL: msvc` comments, which accurately describe runtime failures for x86 and x64.
# However, for ARM and ARM64, they successfully compile, then we don't run them.
# Our test harness properly handles the ambiguity of whether a FAIL line in this file means "fails to compile"
# or "fails to run", combined with the `build_only` setting that we use for ARM and ARM64.
# The upstream logic that parses `// XFAIL: msvc` bypasses this, so it's interpreted as "this always fails",
# so compilation success for ARM and ARM64 is reported as unexpectedly passing.
# The test harness should be fixed to treat `// XFAIL: msvc` the same way that FAIL lines here are treated.
# In the meantime, because this is platform-dependent and we don't have a way to express that in this file,
# we need to mark these tests as SKIPPED.

@StephanTLavavej
Copy link
Member

Thank you, this is awesome! 😻

@StephanTLavavej StephanTLavavej self-assigned this Jan 8, 2024
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 532672c into microsoft:main Jan 9, 2024
37 checks passed
@StephanTLavavej
Copy link
Member

Thanks again for investigating these failures! 😻 🕵️ 💡

@cpplearner cpplearner deleted the analyze-skipped branch January 9, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels
test Related to test code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cpplearner @StephanTLavavej