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

Add a testing script and a github workflow #897

Merged
merged 5 commits into from
Jan 9, 2024

Conversation

jarzec
Copy link
Contributor

@jarzec jarzec commented Dec 20, 2023

Following this comment Here is my second attempt at a PR with workflow running regression tests.

It is based on a bash script that allows for running on different systems (Linux, MacOS, Windows - through git bash) using different compilers.
I found a bash script to be the most flexible and easily understandable solution that adds no extra dependencies to the project. It should be fairly easy to maintain esp. given the number of people interested in the project 😉.

Below is the long story of how useful the script can be.

Running tests locally

The script should be executed where it is placed - in regression-tests dir (as shown i the YAML file).
It can be used to run all the tests or only a subset specified through a command line argument as follows:

  • bash run-tests -c g++-10 - runs all the tests using g++12,
  • bash run-tests -c clang++-13 -t mixed-bounds-check.cpp2,pure2-stdio.cpp2 - runs only mixed-bounds-check.cpp2 and pure2-stdio.cpp2 using clang-13; this is very useful when working on particular test(s), e.g. adding a new one.

Running on Windows

As mentioned before, the script was written in a way to allow it to be simply run on Windows. This is shown at the very end of .github/workflows/regression-tests.yml. Here it comes for convenience:

"C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\vcvars64.bat" && ^
git config --local core.autocrlf false && ^
cd regression-tests && ^
bash run-tests.sh -c cl.exe

The first line sets up the environment. It has to be adapted for the VS version used.
The second line is a convenience to avoid issues of line feeds.
For the last line to work Git Bash has to be in the PATH.
The above can be turned into a .bat script.

Updating tests

The script simply uses git to check for regression. In case of error it prints the output of git diff. This is very helpful for updating tests for machines you don't have access to. I am personally working on the WSL Ubuntu on my Windows. I was able to create some test update PRs, like #894, thanks to the script and older versions of the proposed workflow.

  • To update tests locally you just need to run the script and commit the modified files.
  • To update tests on systems you don't have access to you can rely on the GitHub workflow. E.g. At the time of writing mixed-bounds-check.cpp2 fails on main on MacOS as can be seen here. I was able to create Update bounds check apple-clang-14 test #896 by copying the output of git diff printed by the script, pasting it into a patch file and applying the patch, as in:
    $ cat > update-test.patch; git apply update-test.patch; rm update-test.patch;
    <pasted diff output> + Ctrl+D
    

In fact diff outputs of multiple failing tests can be put in a single patch file. That is what I did for #894 😉.

Error reporting

  • The script reports regression errors in terms of non-empty git diff of all the respective generated output, code, stdout, ... as committed for the given compiler.
  • If such a file is generated during the run but is not checked into git an error is reported.
  • Compilation failures with errors matching the checked-in output files are not treated as error but are reported in the final summary. This allows for easier identification of compiler issues (shortcomings in terms of C++20 compliance) of each compiler.

Note: This PR adds two files regression-tests/test-results/pure2-stdio-with-raii.files and regression-tests/test-results/pure2-stdio.files. These contain the names of output files (one per line) created by the respective compiled binary. Those files allow the script to also check content of those output files that themselves need to be checked into git.

Adding/evaluating a new compiler

The script allows for easy generation of reference output for a given new compiler. To do that a new empty directory has to be added (e.g. regression-tests/test-results/clang-15), the compiler has to be included in the script (this has already been done for Clang-15 in line 87), and the script needs to be run (e.g. bash run-tests.sh -c clang++15 to continue the example). The script will report errors for every test but will generate all the files none the less. These files can be committed and a new compiler is added.
Clang-15 has been used as reference in the example as all current tests compile correctly using the 15.0.7 version that can be installed on LTS Ubuntu. I will propose addition of tests for this compiler in another PR as it is the only one already available in Ubuntu 2024 that allows to correctly generate, compile and run all regression tests for C++20 (tested on WSL on Windows 11).
The fact that I was able to quickly check that shows how useful the script is.

@jarzec
Copy link
Contributor Author

jarzec commented Dec 20, 2023

Note that the MacOS run is indicated as failing for this PR. This is solved in #896, as mentioned above.

fail-fast: false
matrix:
os: [ubuntu-latest]
compiler: [g++-10, g++-13]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clang-12 can in principle be installed and used on the runner. However, I had issues analogous to those when I tried to use it, hence it is not included yet. On my Ubuntu I have no issues using that version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, you often need to use the libc++ shipped with Clang, or an older libstdc++ version.
That's specially true if you live at HEAD.

################
# Get the directory with the exec outputs and compilation command
if [[ "$cxx_compiler" == *"cl.exe"* ]]; then
compiler_cmd='cl.exe -nologo -std:c++latest -MD -EHsc -I ..\include -I ..\..\..\include -experimental:module -Fe:'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 2 include paths are necessary due to the fact that compilation has to be requested in two different folders due to the current setup. This could be improved, but will require small modification in the tests result files.
With that modification, if compilation of the generated C++ is performed in the regression-tests directory, the error outputs from cppfront are more detailed and include pointers into the cpp2 source. This is because the cpp2 files are found in that directory.
I didn't want to modify a lot of files in this PR. I can create another PR that will explain better what I mean here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you use an absolute include path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried. The thing is the path may show up in test results, on compilation errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right.
Relative path is what the current scripts in the main branch are using.

# For some tests the binary needs to be placed in "$exec_out_dir"
# For that reason the compilation is done directly in that dir
# The source is temporarily copied to avoid issues with bash paths in cl.exe
(cd $exec_out_dir; \
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 compilation command could be greatly simplified after a modification discussed in this comment.

Copy link
Contributor

@JohelEGP JohelEGP left a comment

Choose a reason for hiding this comment

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

Just a few spelling fixes in comments, and a link to a related issue at https://github.com/modern-cmake/cppfront/.

regression-tests/run-tests.sh Outdated Show resolved Hide resolved
regression-tests/run-tests.sh Outdated Show resolved Hide resolved
regression-tests/run-tests.sh Outdated Show resolved Hide resolved
regression-tests/run-tests.sh Outdated Show resolved Hide resolved
regression-tests/run-tests.sh Outdated Show resolved Hide resolved
if [[ $compilation_result -ne 0 ]]; then
# Workaround an issue with MSVC mising std modules
if cat $expected_src_compil_out | grep -q "error C1011"; then
echo " Skipping further checks due to missing std modules support"
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think I should include the issue link in a comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this is just informational.

@jarzec jarzec requested a review from JohelEGP December 20, 2023 14:07
@jarzec
Copy link
Contributor Author

jarzec commented Dec 20, 2023

Just a few spelling fixes in comments, and a link to a related issue at https://github.com/modern-cmake/cppfront/.

Thanks a lot, I committed all your suggestions.

@hsutter
Copy link
Owner

hsutter commented Jan 2, 2024

Thanks! I see the MacOS run is having trouble with a few:

Tests with failing compilation step:        7
    mixed-captures-in-expressions-and-postconditions.cpp2
    mixed-function-expression-and-std-for-each.cpp2
    mixed-function-expression-and-std-ranges-for-each-with-capture.cpp2
    mixed-function-expression-and-std-ranges-for-each.cpp2
    mixed-function-expression-with-pointer-capture.cpp2
    mixed-function-expression-with-repeated-capture.cpp2
    pure2-type-and-namespace-aliases.cpp2
Failed tests:        1
    mixed-bounds-check.cpp2

Is there some further tweaking needed for these?

@jarzec
Copy link
Contributor Author

jarzec commented Jan 3, 2024

Thanks! I see the MacOS run is having trouble with a few:

Tests with failing compilation step:        7
    mixed-captures-in-expressions-and-postconditions.cpp2
    mixed-function-expression-and-std-for-each.cpp2
    mixed-function-expression-and-std-ranges-for-each-with-capture.cpp2
    mixed-function-expression-and-std-ranges-for-each.cpp2
    mixed-function-expression-with-pointer-capture.cpp2
    mixed-function-expression-with-repeated-capture.cpp2
    pure2-type-and-namespace-aliases.cpp2
Failed tests:        1
    mixed-bounds-check.cpp2

Is there some further tweaking needed for these?

In fact the last run of the github action that was shown in the PR is the one from roughly 2 weeks ago. At that stage the main branch had the issue indicated by the workflow that was fixed in #896. I've learned today that there is no option to ask GitHub to re-run a PR workflow when the base branch is updated. You @hsutter as the project owner can manually rerun the tests: you can click on Details above and you should see a Re-run all jobs button. I believe that will take the most recent version of the base branch.

Another way to have a rebuild is to push something into the PR branch itself, which is what I did. I have a workflow_dispatch event to the ones running the workflow. When this is in main it will be possible to run regression tests manually e.g. on a givet branch. In fact now that is another test failing on MacOS, namely pure2-return-tuple-operator.cpp2. If you look at the run details you can see that the reference files for this tests were not added for MacOS:

    Testing pure cpp2 code: pure2-return-tuple-operator.cpp2
        Generating Cpp1 code
        Compiling generated code
            The compilation message file is not tracked by git:
                test-results/apple-clang-14/pure2-return-tuple-operator.cpp.output
        Executing the compiled test binary
            Execution output file is not tracked by git:
                test-results/apple-clang-14/pure2-return-tuple-operator.cpp.execution

The trouble in this case is that the script does not print the content of the missing files so they cannot be updated just using the results of the script. I will try to fix that soon.

If in your question you were referring to Tests with failing compilation step: 7 I have added this diagnostic to keep track of issues related to compiler compliance with the standard. If i am not mistaken, currently the only compiler having reference data in regression_tests/test-results that is conformant enough to compile all the tests is GCC 13. All others have compilation error outputs committed as expected due to issues with compliance. The proposed bash script helps keeping track of such issues.

@JohelEGP
Copy link
Contributor

JohelEGP commented Jan 3, 2024

If i am not mistaken, currently the only compiler having reference data in regression_tests/test-results that is conformant enough to compile all the tests is GCC 13.

No, GCC 13 fails for some UFCS tests.
I think MSVC is the only one that can compile all of them.

@jarzec
Copy link
Contributor Author

jarzec commented Jan 7, 2024

@hsutter
The PR #919 by @filipsajdak solves the issue with the missing MacOS test files. In its current version the mentioned PR also removes some empty test files that are useful for testing. I have checked that with an updated version of the testing script and added a change request comment.
When #919 is merged I will update this PR.

@hsutter
Copy link
Owner

hsutter commented Jan 7, 2024

Thanks! Merged #919

@jarzec
Copy link
Contributor Author

jarzec commented Jan 8, 2024

@hsutter
I have opened #920 to re-add the 4 files removed in #919. These are necessary for the automated testing to work.

@jarzec jarzec force-pushed the github-test-workflow-5 branch from bf94a5b to 8cc9d81 Compare January 8, 2024 20:04
@jarzec
Copy link
Contributor Author

jarzec commented Jan 8, 2024

@hsutter
I have updated the testing script and rebased my branch on your current main.
As you can see the tests are all green.

@hsutter
Copy link
Owner

hsutter commented Jan 9, 2024

Thanks! Let's give it a try...

@hsutter hsutter merged commit 86fb73b into hsutter:main Jan 9, 2024
14 checks passed
@JohelEGP
Copy link
Contributor

This is a great start!
Thank you very much, @jarzec!

@hsutter
Copy link
Owner

hsutter commented Jan 11, 2024

@jarzec, thanks again! Say, do you happen to know why just a couple of these show as failing on recent PRs? I can see from the details which files they are, but I haven't delved deep enough to figure out why, or how to fix it.

@jarzec
Copy link
Contributor Author

jarzec commented Jan 11, 2024

@jarzec, thanks again! Say, do you happen to know why just a couple of these show as failing on recent PRs? I can see from the details which files they are, but I haven't delved deep enough to figure out why, or how to fix it.

@hsutter I could see that for most PRs one (or a few) of the dirs in test-resutls (related to the compiler(s) the author is using) would have all tests passing, while others would have tests failures. Authors would likely run and update all tests for their own compiler(s). However, it is difficult/impossible to update test for other platforms/compilers.
In such a case a review of the failures reported by GItHub should still be performed as they might indicate actual problems with the PR. However, most likely the diffs for the other platforms/compilers can be just applied through patches. In this sense the GItHub workflow will be a great help for keeping all tests up to date.

The regression tests workflow is set up in such a way that tests are run for each push. This means that when you are working on branch for a new PR you can see the results of running tests on all platforms after every git push, i.e. even before a PR is open. This can be seen in the Actions section on GitHub:
image
This way you can update tests also for other platforms and be sure that when you open a PR all tests will be green.

One last thing is that it is not possible to have GitHub re-run tests on a PR when the target branch gets updated (it only happens if the PR branch istelf is updated).
For that reason I would suggest manually re-running tests on each PR just before it is merged. This can be done by clicking Details of the PR regression test build:
image
and clicking Re-run all jobs in the action page that opens.

Maybe it would be a good idea to create a page on how to use the GihHub actions and the testing script itself. I think it can facilitate testing and test updates quite a bit.

@jarzec jarzec deleted the github-test-workflow-5 branch January 16, 2024 20:25
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.

3 participants