Skip to content

Memory usage performance test [15785] #170

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

@jparisu jparisu temporarily deployed to codecov September 30, 2022 11:05 Inactive
@jparisu jparisu temporarily deployed to codecov September 30, 2022 11:05 Inactive
@jparisu jparisu changed the title Memory usage performance test Memory usage performance test [15785] Oct 3, 2022
@jparisu jparisu temporarily deployed to codecov October 4, 2022 10:39 Inactive
@jparisu jparisu marked this pull request as draft October 6, 2022 06:26
@jparisu jparisu temporarily deployed to codecov October 25, 2022 06:14 Inactive
@mergify mergify bot mentioned this pull request Oct 25, 2022
@jparisu jparisu temporarily deployed to codecov October 25, 2022 07:09 Inactive
@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.42%. Comparing base (74f5197) to head (d7bd7bf).
Report is 70 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #170   +/-   ##
=======================================
  Coverage   58.42%   58.42%           
=======================================
  Files          33       33           
  Lines        4421     4421           
  Branches     2352     2352           
=======================================
  Hits         2583     2583           
  Misses         54       54           
  Partials     1784     1784           

☔ 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.

@jparisu jparisu temporarily deployed to codecov October 25, 2022 12:10 Inactive
@MiguelCompany
Copy link
Member

@EduPonz I think I don't have enough knowledge about bash scripting to review this one ...

@MiguelCompany
Copy link
Member

@jparisu Please rebase this one

jparisu added 2 commits October 27, 2022 07:46
@EduPonz EduPonz temporarily deployed to codecov October 27, 2022 05:53 Inactive
@EduPonz
Copy link

EduPonz commented Oct 27, 2022

I have rebased the PR while looking at it. I think we ought to have a discussion about this; right now, it's a draft as it is far from being ready to be incorporated in CI and merged. The pain points I see with the current for merging as a test are:

  • The test creates a dependency on a specific Fast DDS example, which may not be present
  • CMakeLists.txt to declare the test for ctest
  • Discussion on passing/failing criteria
  • Script documentation & help guide
  • CSV documentation

Another possibility would be to add the script as a resource from which users can benefit, but I'm really attracted to the idea of having this kind of stress tests.

@jparisu
Copy link
Contributor Author

jparisu commented Oct 27, 2022

Another possibility would be to add the script as a resource from which users can benefit, but I'm really attracted to the idea of having this kind of stress tests.

This test was never meant to be added in the CI. This was intended to be a manual test to have a partial idea of what is the memory usage of an execution.
In order to automatize it and have a failing criteria I think it could not be done as results highly depend on the architecture, OS and state of the machine where tests are running. I think it would be better to have it as a helper script rather than an actual test (does Fast DDS have memory usage automatized tests with failing criteria?).

PD: I personally think we have bigger problems that having a stress test.

@jparisu
Copy link
Contributor Author

jparisu commented Oct 27, 2022

This is an image of the results obtained by running this test:
image

Signed-off-by: jparisu <[email protected]>
@jparisu jparisu temporarily deployed to codecov October 27, 2022 10:40 Inactive
@EduPonz
Copy link

EduPonz commented Oct 28, 2022

I completely agree. I'd leave this as a draft for now and we'll see in the future

@JLBuenoLopez
Copy link
Contributor

JLBuenoLopez commented Mar 16, 2023

@jparisu which is the status of this Draft? Does it make sense to try to merge it? I think we should change the status and assign a milestone so the discussion is had at some point.

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