hsutter / cppfront

A personal experimental C++ Syntax 2 -> Syntax 1 compiler
Other
5.23k stars 224 forks source link

[CI] Fix regression tests #1140

Closed bluetarpmedia closed 1 week ago

bluetarpmedia commented 1 week ago

Update a couple of test-results caused by line number changes in cpp2util.h

DyXel commented 1 week ago

I am curious, is there something we could do to avoid these repeating PRs? My only guess is that it's only possible if Herb decides to do every change through a PR (and thus run the CI before merging), which might not sound amusing to him 😅

hsutter commented 1 week ago

I am curious, is there something we could do to avoid these repeating PRs? My only guess is that it's only possible if Herb decides to do every change through a PR (and thus run the CI before merging), which might not sound amusing to him 😅

Interesting... so if I did all my commits as PRs, that would update the regression tests CI? (I appreciate the help with the CI but don't myself grok its inner workings.)

DyXel commented 1 week ago

CI runs all the available compilers and creates git patches that you can download and apply locally, you can apply those changes in that same unmerged PR and avoid "breaking" the main branch.

Another option I thought of, is to account for your current workflow and create a bot that automatically opens these kind of PRs after commits have been pushed to main. This would have less friction for you, and also don't have users wasting valuable time updating tests every time. But of course we would actually need to write and deploy that bot.

hsutter commented 1 week ago

create a bot that automatically opens these kind of PRs after commits have been pushed to main

That sounds promising, and would give a way to review the impact, albeit post-commit.

A third option would be to check in only the tests that I run myself (including that I run them on others' PRs before merging): Those are currently MSVC latest, GCC 10 and 14, and Clang 12, and for completeness I could add Clang 18. That would cover the lowest-supported and pretty-most-recent versions of GCC and Clang. The major thing missing is it would not cover Apple Clang, which was the original motivation to check in additional builds I wasn't doing myself (because it requires a Mac, and I'm not currently set up for that)... but if I always did { MSVC, GCC 10, GCC latest, Clang 12, and Clang latest }, and in addition we only had Apple Clang latest test results supplied some other way, that would still reduce the number of files and keep all but one of the regression tests accurate.

WDYT?

DyXel commented 1 week ago

Personally I don't have strong feelings one way or another, and I think this is more of a question for @bluetarpmedia and co., to me it just feels kind of bad seeing them open a PR to update tests after every change. #1137 should help with that, since only "compiler negative cases" (that is, cases that should fail to compile on the C++ compiler) would need to be updated.

bluetarpmedia commented 4 days ago

I've been thinking about a couple things but hadn't put them down in writing yet so this is a good opportunity: First, I've been wondering if we should drop some of the compiler configurations for the regression tests, firstly to simplify the amount of files but secondly because I'm not sure we're getting much value from some of them.

Here's what I'm thinking:

Compiler C++ Standard Comment
Apple Clang 14 C++2b ✅ Keep: Min supported Apple Clang? (I can't remember if cppfront works with earlier versions, or maybe it's the min supported on GitHub runners)
Apple Clang 15 C++2b ❌ Drop: Doesn't seem to provide much value compared to above
Clang 12 C++20 ✅ Min supported Clang in C++20 mode
Clang 15 C++20 ❌ We already test C++20 with Clang 12 above
Clang 15 with libc++ C++20 ❌ This is currently the only Ubuntu Clang test using libc++, but I'd drop it in favour of changing Clang 18 below.
Clang 18 C++20 ✅ Change this to C++2b and use the libc++ standard library
GCC 10 C++20 ✅ Min supported GCC in C++20 mode
GCC 13 C++2b ❌ Doesn't appear to provide much value compared to GCC 14 below
GCC 14 C++2b
MSVC 2022 C++20 ✅ Min supported MSVC in C++20 mode
MSVC 2022 C++latest

So after the change there'd be:

I think that's close to what Herb suggested above, except I think it's also valuable to test the min supported MSVC in C++20 mode and not only the C++latest?


The other thing that's been on my mind is regarding the git diff conflicts we get with cpp2util.h when the only difference is that the line and/or column number has changed.

What do you think about stripping out the line & column numbers from the compiler output before running the git diff (and also committing that version of the output sans numbers as the expected results)?

So lines like this:

../../../include/cpp2util.h:10005:33: error: expected unqualified-id

would be edited to:

../../../include/cpp2util.h: error: expected unqualified-id

and then run the git diff on that.

DyXel commented 4 days ago

First, I've been wondering if we should drop some of the compiler configurations for the regression tests, firstly to simplify the amount of files but secondly because I'm not sure we're getting much value from some of them.

I guess in principle is not that bad of an idea: Just test the minimally supported compiler set and hope implementers didn't change anything or introduce a regression. My problem with this is that the amount of of files is not really about the target count, but about our current regression test implementation.

For the 2nd, I am not too sure... Its reasonable to think that if you change the file and now it errors out in a different place, that you'd need to refresh the regression tests, to me the problem isn't the conflict, is that its maybe too cumbersome to update them at the moment.