Skip to content

Conversation

@mcbarton
Copy link
Collaborator

@mcbarton mcbarton commented Nov 6, 2025

Description

Please include a summary of changes, motivation and context for this PR.

Fixes # (issue)

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Added/removed dependencies
  • Required documentation updates

@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.05%. Comparing base (5860e95) to head (d47866d).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #405   +/-   ##
=======================================
  Coverage   82.05%   82.05%           
=======================================
  Files          21       21           
  Lines         858      858           
  Branches       89       89           
=======================================
  Hits          704      704           
  Misses        154      154           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mcbarton
Copy link
Collaborator Author

mcbarton commented Nov 6, 2025

I am not sure what is going wrong with using Valgrind on Ubuntu arm runners in the ci (see https://github.com/compiler-research/xeus-cpp/actions/runs/19150317221/job/54738556792?pr=405 ) . This Valgrind error doesn't occur when I run same command locally (and how I managed to produce the Valgrind suppression file. I don't know how to interpret the suppression files, so unsure if there is anything in there that we don't want to suppress.

@anutosh491
Copy link
Collaborator

anutosh491 commented Nov 7, 2025

Why is this PR needed in the first place ?

P.S : Asking as someone who is clueless about Valgrind.

@mcbarton mcbarton force-pushed the Add-Valgrind-check-ci branch from 9a66c1c to 4c0ec52 Compare November 10, 2025 14:39
@mcbarton mcbarton changed the title [wip] Run C++ tests in Valgrind ci [ci] Run tests under Valgrind Ubuntu x86 Nov 10, 2025
@mcbarton mcbarton force-pushed the Add-Valgrind-check-ci branch from 4c0ec52 to d47866d Compare November 10, 2025 14:41
@mcbarton
Copy link
Collaborator Author

mcbarton commented Nov 10, 2025

Why is this PR needed in the first place ?

P.S : Asking as someone who is clueless about Valgrind.

The truthfull reason as to why I added this to the ci is to be consistent with all of compiler researches other repos, which run the tests under Valgrind. I cannot list the benefits of Valgrind as I am still in the process of teaching myself how to use it effectively. I for example cannot tell you if I am suppressing anything that we shouldn't. @vgvassilev @Vipul-Cariappa or @aaronj0 though may be able to enlighten you as to why its used in all the other repos, and help you with a review (once they have bandwidth).

I have limited this PR to just running the tests under Valgrind for Ubuntu x86, as running the tests under Valgrind on Ubuntu arm caused an illegal instruction error from Valgrind which doesn't happen locally, so possibly a bug with the runner. I consider this PR now ready for review.

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