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

.github: add initial workflow to run tests #38

Merged
merged 18 commits into from
Jun 27, 2024
Merged

Conversation

ee7
Copy link
Contributor

@ee7 ee7 commented Jun 18, 2024

Add an initial workflow to run dev run in CI on the Linux and macOS runners. The output currently needs to be inspected manually, but this PR gives us a base to build on and could already help catch regressions like #37 (and make it easier to verify fixes on other platforms).

Note that currently:

  • The dev run command exiting with an exit status of 0 does not imply that the tests pass.
  • The workflow indicates success even when dev run segfaults.

We can improve such things over time.

Also use the latest stable GCC release (GCC 14.1) in the Linux job to help make newer compiler warnings more visible.

Closes: #34 (we can make follow-up tickets for e.g. the exit status)
Refs: #35
Refs: #37


To-do:

ee7 added 2 commits June 18, 2024 20:23
The `dev run` command exiting with an exit status of 0 does not
currently imply that the tests pass. However, adding this workflow does
at least allow us to enforce in CI that the commands to build and test
don't exit non-zero.
Previously, the macOS GitHub Actions build errored with:

    meson.build:170:14: ERROR: C shared or static library 'crypto' not found

because Meson's find_library() calls weren't searching where the
libraries are installed on the macOS runner. For now, just add some
symlinks, so that all the CI-related setup is in one place.
@ee7 ee7 marked this pull request as ready for review June 19, 2024 12:10
@ee7 ee7 requested a review from viega June 19, 2024 12:10
@ee7 ee7 mentioned this pull request Jun 19, 2024
.github/workflows/test.yml Outdated Show resolved Hide resolved
ee7 added 2 commits June 21, 2024 11:55
I sometimes prefer a single step when the same conceptual operation is
performed on multiple OSes. But Miro prefers this, and I don't mind.
@ee7 ee7 requested a review from miki725 June 21, 2024 10:31
Copy link
Contributor

@viega viega left a comment

Choose a reason for hiding this comment

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

It doesn't seem to deal w/ the exit code / crashes. For instance, it's certainly not working on Linux right now based on the runner output, but it still passes.

Also, since I'm generally not working on a Linux box and switching is a bit of a PITA, would appreciate some insight there.

@miki725
Copy link

miki725 commented Jun 21, 2024

libcon4m/dev

Lines 128 to 139 in 8333004

function libcon4m_run_tests {
shift
TARGET=$(cat .meson_last)
mason_build ${TARGET} --buildtype=${TARGET}
log Running test binary for target: $(color green '[==' $TARGET '==]')
cd $TARGET
./c4test $@
cd ..
}

looks like exit code is not honored in the dev script in general. not sure how that works on mac at all. we can either set -e in the script to bail out on any non-successful commands or explicitly handle the case when ./c4test fails

@ee7
Copy link
Contributor Author

ee7 commented Jun 21, 2024

Yes, my intention for this PR was simply to get dev run into CI. It's true that alone currently isn't as useful as it should be, because the dev script has some problems. But let's handle that in a separate PR, which should make dev run exit non-zero when c4test exits non-zero. That will fix the behavior of this workflow without any changes to the workflow itself.

I would've already added set -e to the script, but we can't do that alone because e.g. we currently have this typo:

libcon4m/dev

Line 133 in 8333004

mason_build ${TARGET} --buildtype=${TARGET}

and changing mason to meson won't work because meson_build always exits the script:

libcon4m/dev

Lines 66 to 73 in 8333004

cd ..
if [[ ${1} != "debug" ]]; then
exit $CODE
fi
if [[ $CODE -ne 0 ]]; then
exit $CODE
fi

so just adding set -e and fixing the typo means that dev run will never run c4test.

This is what I meant when I mentioned the typo yesterday, and with:

Note that currently:

  • [...]
  • The workflow indicates success even when dev run segfaults.

[...]

Closes: #34 (we can make follow-up tickets for e.g. the exit status)

@viega
Copy link
Contributor

viega commented Jun 21, 2024

I don't understand the need to be rushing it in if it isn't where it needs to be. There's no need to overengineer the build; I don't want to do more than the bare minimum necessary (for instance with the dev script), because they're not a priority yet. Breaking this up feels like an invitation to bikeshed, and I would rather stay focused.

ee7 added 2 commits June 21, 2024 19:45
Before this commit, the new test workflow would indicate success when it
shouldn't (for example, when one of the tests produced a segfault).

This happened because `dev run` would exit with an exit code of 0 even
when `c4test` didn't. For now, make the `dev run` exit with the
exit code of `c4test`.

Note that we cannot currently add `set -e` in this script without making
other changes.
Set the CON4M_DEV env var. From the commit message of the commit that
added the initial test runner [1]:

    Also, the harness looks for two environment variables:

    1) The tremendous amount of debug info now only shows up if you have CON4M_DEV set (ignores value).
    2) The default search location is pulled from CON4M_TEST_DIR if provided.

Internally, this env var sets dev_mode in src/tests/test.c:

    514:    if (c4m_get_env(c4m_new_utf8("CON4M_DEV"))) {
    515:        dev_mode = true;
    516:    }

[1] 8333004, 2024-06-20, "Initial test runner"
@ee7
Copy link
Contributor Author

ee7 commented Jun 21, 2024

I don't understand the need to be rushing it in if it isn't where it needs to be

It's just that:

  • I prefer PRs to be tightly focused.
  • I considered adding our first workflow file, and altering the behavior of dev run, to be issues that can be considered independently.
  • It already seemed handy, given that c4test on main currently produces segfaults that you don't reproduce locally.

But fair enough if we don't want to have, even for a momentary first implementation, a workflow that indicates success when the tests clearly error.

Breaking this up feels like an invitation to bikeshed

I can change dev in this PR (and have done so with 4729312), but doing it here means that any objection to those changes blocks this PR rather than another one. But it sounds like this PR was blocked anyway.

For instance, it's certainly not working on Linux right now based on the runner output, but it still passes.

Also, since I'm generally not working on a Linux box and switching is a bit of a PITA, would appreciate some insight there.

I've opened #41 including a stack trace and CON4M_DEV debug output, and set the CON4M_DEV env var in the workflow in this PR. Does that help?

@ee7
Copy link
Contributor Author

ee7 commented Jun 24, 2024

Just to document the current state in the latest Linux CI runs (see log): the segfaults are for basic11 and basic18:

[-- libcon4m --] Running test: [== basic11.c4m ==]
./dev: line 143:  3776 Segmentation fault      (core dumped) ${HERE}/c4test $ITEM
[-- libcon4m --] Running test: [== basic18.c4m ==]
./dev: line 143:  3866 Segmentation fault      (core dumped) ${HERE}/c4test $ITEM

and the other failures are for basic14, basic15, and basic16:

FAIL: test /home/runner/work/libcon4m/libcon4m/tests/basic14.c4m: output mismatch.
Expected output
12
12
7
7
Actual
12
12
12
12
FAIL: test /home/runner/work/libcon4m/libcon4m/tests/basic15.c4m: output mismatch.
Expected output
4
Actual
0
[-- libcon4m --] Running test: [== basic16.c4m ==]
Runtime Error: length cannot be < 0 for string initialization
Stack Trace:
    module basic16:15
info: Compiling: /home/runner/work/libcon4m/libcon4m/tests/basic16.c4m
info: Done processing: /home/runner/work/libcon4m/libcon4m/tests/basic16.c4m
****STARTING PROGRAM EXECUTION*****
****PROGRAM EXECUTION FINISHED*****

FAIL: test /home/runner/work/libcon4m/libcon4m/tests/basic16.c4m: program expected output but did not compile. Expected output:
 Hello,

viega and others added 5 commits June 26, 2024 23:39
…casional crash that was due to occasional u16 laying in memory in such a way as to look like a valid heap pointer. I need to go back and do ptr scanning for everything, but am thinking about whether it is more automatable to produce the ptr maps, e.g., via lib clang. Until then, I've made some of the data types wide.
@ee7 ee7 self-assigned this Jun 27, 2024
@ee7
Copy link
Contributor Author

ee7 commented Jun 27, 2024

This was approved by #49 (comment). Merging.

@ee7 ee7 merged commit 14604ad into main Jun 27, 2024
2 checks passed
@ee7 ee7 deleted the ee7/add-initial-test-workflow branch June 27, 2024 12:14
@ee7 ee7 mentioned this pull request Jul 2, 2024
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.

c4test segfaults with e.g. basic1.c4m enable tests in ci
3 participants