Skip to content

Conversation

@DomPeliniAerospike
Copy link
Collaborator

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.25%. Comparing base (4aea102) to head (586995f).
⚠️ Report is 28 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #871      +/-   ##
==========================================
+ Coverage   82.45%   83.25%   +0.80%     
==========================================
  Files          99       99              
  Lines       14496    14347     -149     
==========================================
- Hits        11952    11945       -7     
+ Misses       2544     2402     -142     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

if: ${{ inputs.massif }}

- run: echo VALGRIND_ARGS="--leak-check=full" >> $GITHUB_ENV
- run: echo VALGRIND_ARGS="--leak-check=full --gen-suppressions=all --suppressions=full_suite.supp --num-callers=350 " >> $GITHUB_ENV
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, it might be helpful to comment explaining why we are setting --num-callers=350 and --gen-suppressions=all, for future reference

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added README.md file explaining the suppression workflow

Renamed suppression file
Added script for extracting suppressions
Removed valgrind-python.supp suppresisons
@DomPeliniAerospike DomPeliniAerospike marked this pull request as ready for review November 21, 2025 22:45
Copy link
Collaborator

@juliannguyen4 juliannguyen4 left a comment

Choose a reason for hiding this comment

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

Hey Dominic, I left some suggestions and questions

```

--error-limit=no: Disable the limit of the numbers errors that can be reported
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
--error-limit=no: Disable the limit of the numbers errors that can be reported
`--error-limit=no`: Disable the limit of the numbers errors that can be reported


--error-limit=no: Disable the limit of the numbers errors that can be reported

--leak-check=full: Show details for each individual memory error
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
--leak-check=full: Show details for each individual memory error
`--leak-check=full`: Show details for each individual memory error


--leak-check=full: Show details for each individual memory error

--gen-suppressions=all: generate suppressions blocks alongside all memory errors
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
--gen-suppressions=all: generate suppressions blocks alongside all memory errors
`--gen-suppressions=all`: generate suppressions blocks alongside all memory errors


--gen-suppressions=all: generate suppressions blocks alongside all memory errors

--num-callers=350: include 350 stack frames of context for each error and suppression.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
--num-callers=350: include 350 stack frames of context for each error and suppression.
`--num-callers=350`: include 350 stack frames of context for each error and suppression.


Setting --num-callers=350 forces the total number of suppressions frames to be below 500 and avoids this error

## 2. Download the results of the valgrind run step
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## 2. Download the results of the valgrind run step
## 2. Download the logs for the valgrind job

?

total_blocks = 0

# Matches the time stamp
ts_re = re.compile(r"^\d{4}-\d{2}-\d{2}T\S+\s+(.*)$")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ts_re = re.compile(r"^\d{4}-\d{2}-\d{2}T\S+\s+(.*)$")
timestamp_pattern = re.compile(r"^\d{4}-\d{2}-\d{2}T\S+\s+(.*)$")

Might be better to rename ts_re to timestamp_pattern?

line = raw_line.rstrip()

# Strip timestamp prefix if present
m = ts_re.match(line)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
m = ts_re.match(line)
ts_match = ts_re.match(line)

Rename m to ts_match?

line = m.group(1)

if line.startswith("{"):
inside_block = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
inside_block = True
inside_suppressions_block = True

# Only keep if 2nd line matches required suppression name
REQUIRED = "<insert_a_suppression_name_here>"

if len(current_block) > 1 and current_block[1].strip() != REQUIRED:
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we pass in a suppressions file when running valgrind, and the suppressions file contains a block with a named suppression, in the logs does valgrind print a suppressions block with that name? I'm not sure when this condition would be true

indented_block = "\n".join(lines)
f.write(indented_block + "\n\n")

print(f"Original number of suppressions: {total_blocks}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
print(f"Original number of suppressions: {total_blocks}")
print(f"Number of suppressions (including duplicates): {total_blocks}")

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.

4 participants