Skip to content

Fix tests and make them platform-independent #556

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

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

jarzec
Copy link
Contributor

@jarzec jarzec commented Jul 22, 2023

  • Update tests that finish through std::terminate.
    • Add std::set_terminate(std::abort); to avoid platform-dependent output on termination.
  • Move regression-tests/test-results/apple-clang-14/ to regression-tests/test-results/clang-14/.
    • After the mentioned update the results are the same for clang++-14 on Linux and clang++ (which is v14) on macOS.
    • Update some outdated reference data files for clang-14.
  • Remove regression-tests/test-results/clang-12/ as the results of using clang-12 are identical to those for clang-14.

Results of running the tests on:

  • Ubuntu 22.04 with:
    • g++-12 (using reference data for gcc-13; v13 is not available by default in Ubuntu 22.04, but the results are exactly the same for v12)
    • clang++-12 (using reference data for clang-14)
    • clang++-14
  • macOS with

NOTE: One delicate thing about macOS is that some parts of the std library are not implemented (see the detailed compilation errors in the mentioned run on macOS). The test execution bash script I am using to run the test in the mentioned GitHub workflow is detecting compilation errors of the type: "error: no member named '.*' in namespace 'std" and simply skips further testing rather than causing a test failure. A Summary of skipped/failed tests is reported at the end.
My rationale here was that C++ implementation shortcomings should not raise cppfront errors.

I will be happy to create a PR with the bash script I used to run the tests or even the GitHub workflow to execute the tests on pushes/PRs if this is deemed useful.

@@ -1,2 +1 @@
Bounds safety violation: out of bounds access attempt detected
libc++abi: terminating
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the platform-dependent message (c.f. the one on Ubuntu below) removed thanks to std::set_terminate(std::abort);.

@@ -1,2 +1 @@
Bounds safety violation: out of bounds access attempt detected
terminate called without an active exception
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the platform-dependent message (c.f. the one on macOS above) removed thanks to std::set_terminate(std::abort);.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the output of macOS clang++ --version. I wasn't sure what to do with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was not detected as move because it was outdated. The original is empty, while the current version of the test prints values to avoid unused warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned above, this file was outdated: unused warnings were fixed by printing values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file had compilation issues due to shortcomings of std implementation on macOS (error: no member named 'pmr' in namespace 'std').
More tests are currently failing on macOS with no member named 'for_each' in namespace 'std::ranges' - see the description of this PR.

@hsutter
Copy link
Owner

hsutter commented Jul 23, 2023

Thanks! I think the core of this PR is the three std::set_terminate calls, right?

I'd rather not delete all the Clang-12 regression test results and change them to Clang 14... I'd like to keep the checked-in ones on 12 to make sure things continue to run on 12.

Would you please trim the PR to not update the test results? Then after merge I can just re-run the regressions on my system (staying on the compilers I'm using) which will reduce the diffs and not change the checked-in Clang regression version.

Thanks1

@jarzec
Copy link
Contributor Author

jarzec commented Jul 23, 2023

Sure, can do. I just noticed that if they are up to date the content of clang-14 and clang-12 directories is identical.
I will bring the apple-clang-14 up to date. I will add the compilation issue results for the 6 tests that fail due to incomplete std implementation (one of those tests already had those failures included in expected test results).
Would that work for you?

@hsutter
Copy link
Owner

hsutter commented Jul 23, 2023

I think so -- so just add the set_terminate calls, and update the Apple directory? SGTM, thx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants