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.

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

tests: Avoid dialog boxes, prevent stdout from being lost #906

Merged
merged 5 commits into from
Jun 20, 2020

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Jun 18, 2020

  • Fix tests/std: Failing asserts emit assertion dialog boxes #781 (dialogs) and fix tests: log files aren't capturing test stdout #789 (stdout).
    • Add tests/std/include/force_include.hpp, derived from libcxx's msvc_stdlib_force_include.h (which I contributed).
      • Add /FI to tests/std/tests/prefix.lst.
      • Update tests/tr1/lit.site.cfg.in so it can access tests/std/include. (This is lazy/expedient. In the future, we might want to have a common test directory, or we might want to upstream this enhanced TestEnvironmentPreparer to libcxx for direct consumption.)
      • Add /FI to tests/tr1/prefix.lst.
    • _CrtSetReportMode and _CrtSetReportFile send assert messages to stderr instead of displaying dialog boxes in debug mode.
    • However, in release mode (/MT and /MD), assert simply calls abort which displays a crash dialog box from Windows Error Reporting. _set_abort_behavior prevents that. (Note 1)
    • Anything printing to stdout (like printf, puts, and cout) might not be captured when we assert/abort because of buffering. setvbuf disables buffering, preventing this. (Note 2)
  • Print randomized test failures to stderr.
    • These are extremely important (as otherwise they aren't reproducible), so I think it's best to explicitly print to stderr instead of relying on the setvbuf fix here.
  • Change wcout to wcerr in P0218R1_filesystem.
    • Similar reasoning: This might also fail at runtime and we really need its output to investigate.
  • Update include_each_header_alone_matrix.lst.
    • Test iso646.h, which is still part of the repo.
    • Don't test CRT headers, which aren't part of the repo.
    • (This was problematic because the force-included header was macroizing errno, which was stomping over our attempt to test errno.h by forming the header name through macro replacement.)
  • NO_TEST_ENVIRONMENT_PREPARER escape hatch.
    • This makes Dev11_0000000_include_each_header_alone cleaner.

I've manually tested each of these changes. Temporarily reverting #800 allowed me to verify that P0218R1_filesystem failure is sent to stderr, and that it doesn't emit assertion dialog boxes in debug mode or crash dialog boxes in release mode. Temporarily partially reverting #724 allowed me to verify that the randomized tests are printing to stderr. (As these changes were systematic, I didn't exercise the other randomized tests.) Finally, I took an ordinary test and added each of cout, puts, and printf to the end, followed by a failed assert, and verified that the test logs were capturing stdout properly.

Note 1: This wasn't a problem for libcxx because we always build it with /MTd:

PM_CL="/EHsc /MTd /std:c++latest /permissive- /FImsvc_stdlib_force_include.h /wd4643"

Note 2: This wasn't a problem for libcxx because it generally doesn't print things to stdout. We primarily experience compiler errors and failed assertions.

@StephanTLavavej StephanTLavavej added the test Related to test code label Jun 18, 2020
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner June 18, 2020 01:42
@@ -3,4 +3,4 @@

RUNALL_INCLUDE ..\universal_prefix.lst
RUNALL_CROSSLIST
PM_CL="/D_ENFORCE_FACET_SPECIALIZATIONS=1 /D_CRT_SECURE_NO_WARNINGS"
PM_CL="/FIforce_include.hpp /D_ENFORCE_FACET_SPECIALIZATIONS=1 /D_CRT_SECURE_NO_WARNINGS"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should note that the importer will need to verify that std/include is made available to tr1 when running in the internal test harness.

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent catch, I forgot that Microsoft-internal src/qa/distrib.yaml would need to be changed. Thanks!

Test iso646.h, which is still part of the repo.

Don't test CRT headers, which aren't part of the repo.
This makes Dev11_0000000_include_each_header_alone cleaner.
@cbezault
Copy link
Contributor

Thanks so much for dealing with this!

@StephanTLavavej StephanTLavavej merged commit 0c34074 into microsoft:master Jun 20, 2020
@StephanTLavavej StephanTLavavej deleted the stderr branch June 20, 2020 07:28
ahanamuk pushed a commit to ahanamuk/STL that referenced this pull request Jun 25, 2020
ahanamuk pushed a commit to ahanamuk/STL that referenced this pull request Jun 26, 2020
ahanamuk pushed a commit to ahanamuk/STL that referenced this pull request Jun 29, 2020
ahanamuk pushed a commit to ahanamuk/STL that referenced this pull request Jul 1, 2020
CaseyCarter pushed a commit to CaseyCarter/STL that referenced this pull request Jul 10, 2020
added test_bidi to range_algo_support

removed fill code

Cleanup ranges test machinery (microsoft#899)

tests: Avoid dialog boxes, prevent stdout from being lost (microsoft#906)

Fixes microsoftGH-781 (dialogs) and fixes microsoftGH-789 (stdout).

updating tests with bidi_range wrapper

fixing tests

initial reverse algo

removed fill code

updating tests with bidi_range wrapper
CaseyCarter pushed a commit to CaseyCarter/STL that referenced this pull request Jul 10, 2020
added test_bidi to range_algo_support

removed fill code

Cleanup ranges test machinery (microsoft#899)

tests: Avoid dialog boxes, prevent stdout from being lost (microsoft#906)

Fixes microsoftGH-781 (dialogs) and fixes microsoftGH-789 (stdout).

updating tests with bidi_range wrapper

fixing tests

initial reverse algo

removed fill code

updating tests with bidi_range wrapper

Fix bad merge in range_algo_support
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.

tests: log files aren't capturing test stdout tests/std: Failing asserts emit assertion dialog boxes
5 participants