This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Add XFAIL LIBCXX-WINDOWS-FIXME in 124 tests that fail in the CI configuration
ClosedPublic

Authored by mstorsjo on Mar 22 2021, 10:53 AM.

Details

Summary

This makes no attempt yet to look into the why/what for each of them,
but makes the CI configuration useful for tracking further regressions.
After looking into each case, they can either be fixed, or converted into
UNSUPPORTED: windows or XFAIL: windows, once the cause is known and explained.

A number of the filesystem cases can be fixed by patches that are currently
in review.

Diff Detail

Event Timeline

mstorsjo created this revision.Mar 22 2021, 10:53 AM
mstorsjo requested review of this revision.Mar 22 2021, 10:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2021, 10:53 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Sounds plausible to me, as long as you remember after this lands that all those other PRs will need to be updated to remove the XFAIL line from the files they affect. (git itself won't detect an explicit merge conflict.)
Should be easy to remember, though, because all the lines-to-be-removed contain the string LIBCXX-WINDOWS-FIXME; right?
I'll withhold an "accept" to see if anyone else has thoughts on the approach; but I'm happy to be a +1 on this if needed.

Sounds plausible to me, as long as you remember after this lands that all those other PRs will need to be updated to remove the XFAIL line from the files they affect. (git itself won't detect an explicit merge conflict.)
Should be easy to remember, though, because all the lines-to-be-removed contain the string LIBCXX-WINDOWS-FIXME; right?

Yeah, I'll keep that in sync with the other patches. For some patches, it's a bit tricky as there might be two patches in review that fixes different issues in the same file, so then the one that lands later needs to do that. But yeah, I'll keep track of it.

ldionne accepted this revision.Mar 22 2021, 2:00 PM
This revision is now accepted and ready to land.Mar 22 2021, 2:00 PM
This revision was landed with ongoing or failed builds.Mar 22 2021, 2:41 PM
This revision was automatically updated to reflect the committed changes.

I'm not sure how temporary this solution is, but there's an issue with the feature-test macro script after this commit. IMO if it's very temporary we don't need to take action otherwise I think we should look at a fix.

libcxx/test/std/language.support/support.limits/support.limits.general/complex.version.pass.cpp
16

This file and the other *.version.pass.cpp are automatically generated by utils/generate_feature_test_macro_components.py. If a developer runs this script these changes will accidentally be reverted. I'm not sure what would be the best fix for now.

I'm not sure how temporary this solution is, but there's an issue with the feature-test macro script after this commit. IMO if it's very temporary we don't need to take action otherwise I think we should look at a fix.

In very general terms, some of the added xfails won't stay for long, while others certainly will stick around for some time. (The fix then either being some change making the tests pass, or changed into UNSUPPORTED: windows or XFAIL: windows after analysis and explanation why.) It partially comes down to review throughput too ;-)

libcxx/test/std/language.support/support.limits/support.limits.general/complex.version.pass.cpp
16

Those particular xfails can be removed with D99213

I'm not sure how temporary this solution is, but there's an issue with the feature-test macro script after this commit. IMO if it's very temporary we don't need to take action otherwise I think we should look at a fix.

In very general terms, some of the added xfails won't stay for long, while others certainly will stick around for some time. (The fix then either being some change making the tests pass, or changed into UNSUPPORTED: windows or XFAIL: windows after analysis and explanation why.) It partially comes down to review throughput too ;-)

I was mainly concerned regarding the XFAILS for the feature-test macros. Obviously it would be great of all can be fixed.
I like the work you do to improve the Windows support!

libcxx/test/std/language.support/support.limits/support.limits.general/complex.version.pass.cpp
16

Since D99213 has landed, could you commit a fix for them?

mstorsjo added inline comments.Mar 25 2021, 10:49 AM
libcxx/test/std/language.support/support.limits/support.limits.general/complex.version.pass.cpp
16

Didn't D99213 already remove them as part of that commit? At least the xfail in std/language.support/support.limits/support.limits.general/complex.version.pass.cpp should have been removed now. Or did I leave stray whitespace changes behind compared with how they're autogenerated?

ldionne added inline comments.Mar 25 2021, 11:00 AM
libcxx/test/std/language.support/support.limits/support.limits.general/complex.version.pass.cpp
16

@Mordante Thanks for noticing this. I missed that during my review because I glanced at the changes and didn't think about the auto-generated files.

@mstorsjo As long as you don't see any diff when you run the script, all's good. If you do see a diff, then we need to fix that.

I'll work on adding a job that catches exactly these issues.

mstorsjo added inline comments.Mar 25 2021, 11:12 AM
libcxx/test/std/language.support/support.limits/support.limits.general/complex.version.pass.cpp
16

CI for enforcing that would be good yeah. And nice that it turned out to be easily fixable...

I don't see any diff after running that script now, so everything should be good now?

ldionne added inline comments.Mar 25 2021, 11:42 AM
libcxx/test/std/language.support/support.limits/support.limits.general/complex.version.pass.cpp
16

No diff = everything's good. Thanks!