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.

Sign up for GitHub

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

Jump to bottom

Fixed tests #244

Merged
merged 4 commits into from
Jul 1, 2021
Merged

Fixed tests #244

merged 4 commits into from
Jul 1, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jun 28, 2021

Signed-off-by: ahcorde [email protected]

🦟 Bug fix

Summary

According with the official Qt documentation.

Warning: The data referred to by argc and argv must stay valid for the entire lifetime of the QGuiApplication object.
In addition, argc must be greater than zero and argv must contain at least one valid character string.

These changes should fix the segmentation faults

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

@ahcorde ahcorde added the bug Something isn't working label Jun 28, 2021
@ahcorde ahcorde self-assigned this Jun 28, 2021
@ahcorde ahcorde requested a review from jennuine as a code owner June 28, 2021 11:47
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jun 28, 2021
@ahcorde
Fixed tests
0dca7f3 
Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde mentioned this pull request Jun 28, 2021
Merged
8 tasks
@codecov
Copy link

codecov bot commented Jun 28, 2021 •
edited
Loading

Codecov Report

Merging #244 (1b8f1c0) into ign-gui3 (3df3ad6) will increase coverage by 51.14%.
The diff coverage is 59.96%.

Impacted file tree graph

@@              Coverage Diff              @@
##           ign-gui3     #244       +/-   ##
=============================================
+ Coverage     14.55%   65.70%   +51.14%     
=============================================
  Files            13       23       +10     
  Lines          1333     2828     +1495     
=============================================
+ Hits            194     1858     +1664     
+ Misses         1139      970      -169
Impacted Files Coverage Δ
include/ignition/gui/Helpers.hh 100.00% <ø> (+100.00%) ⬆️
include/ignition/gui/Plugin.hh 100.00% <ø> (+100.00%) ⬆️
src/ign.cc 37.83% <ø> (+37.83%) ⬆️
src/plugins/grid_3d/Grid3D.cc 28.57% <0.00%> (ø)
src/plugins/image_display/ImageDisplay.cc 23.27% <ø> (ø)
src/plugins/publisher/Publisher.cc 90.78% <ø> (ø)
src/Plugin.cc 57.62% <30.00%> (+57.62%) ⬆️
src/plugins/screenshot/Screenshot.cc 33.33% <33.33%> (ø)
src/plugins/scene3d/Scene3D.cc 42.38% <41.86%> (ø)
src/plugins/key_publisher/KeyPublisher.cc 56.52% <56.52%> (ø)
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fffa6e...1b8f1c0. Read the comment docs.

@ahcorde
fixed tests
b612844 
Signed-off-by: ahcorde <[email protected]>
mjcarroll
mjcarroll reviewed Jun 28, 2021
View reviewed changes
Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Just one nit/question

src/Plugin.cc
@@ -91,7 +91,8 @@ Plugin::Plugin() : dataPtr(new PluginPrivate)
/////////////////////////////////////////////////
Plugin::~Plugin()
{
delete this->dataPtr->pluginItem;
if (this->dataPtr->pluginItem)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessary. The pluginItem is initialized to nullptr, and using delete on a nullptr shouldn't have any impact.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also curious about why this was added. But it shouldn't hurt, so I'm ok leaving it in.

@chapulina chapulina added the tests Broken or missing tests / testing infra label Jun 28, 2021
@chapulina
Copy link
Contributor

These changes should fix the segmentation faults

What segmentation faults? Can you provide more context?

@ahcorde
Copy link
Contributor Author

ahcorde commented Jun 28, 2021 •
edited
Loading

These changes should fix the segmentation faults

What segmentation faults? Can you provide more context?

In general I can see many times UNIT_Application_TEST, UNIT_WorldControl_TEST and UNIT_WorkdStats_TEST` failing because some seg faults. For example: https:/ignitionrobotics/ign-gui/pull/221/checks?check_run_id=2931467916

By the way I ran ASAN inside a docker container and this seg faults always happen. it's not fully solved but it should remove some of them

chapulina
chapulina approved these changes Jun 30, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks for the context, LGTM. I pushed some indentation fixes in 1b8f1c0.

src/Plugin.cc
@@ -91,7 +91,8 @@ Plugin::Plugin() : dataPtr(new PluginPrivate)
/////////////////////////////////////////////////
Plugin::~Plugin()
{
delete this->dataPtr->pluginItem;
if (this->dataPtr->pluginItem)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also curious about why this was added. But it shouldn't hurt, so I'm ok leaving it in.

@chapulina chapulina enabled auto-merge (squash) June 30, 2021 21:15
@chapulina chapulina merged commit 5262650 into ign-gui3 Jul 1, 2021
@chapulina chapulina deleted the ahcorde/fix/tests branch July 1, 2021 01:30
@chapulina chapulina mentioned this pull request Mar 1, 2022
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjcarroll mjcarroll mjcarroll left review comments

@chapulina chapulina chapulina approved these changes

@jennuine jennuine Awaiting requested review from jennuine jennuine is a code owner

Assignees

@ahcorde ahcorde

Labels
bug Something isn't working 🏰 citadel Ignition Citadel tests Broken or missing tests / testing infra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ahcorde @chapulina @mjcarroll