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

537 silence gmock warnings #801

Merged
merged 2 commits into from
Oct 20, 2023
Merged

537 silence gmock warnings #801

merged 2 commits into from
Oct 20, 2023

Conversation

dabele
Copy link
Member

@dabele dabele commented Oct 13, 2023

Changes and Information

Silence some gmock warnings in a unit test by using NiceMock. The default implementation for get_result of the Mock class does the right thing, so silencing the warning is OK.

Merge Request - Guideline Checklist

Please check our git workflow. Use the draft feature if the Pull Request is not yet ready to review.

Checks by code author

  • Every addressed issue is linked (use the "Closes #ISSUE" keyword below)
  • New code adheres to coding guidelines
  • No large data files have been added (files should in sum not exceed 100 KB, avoid PDFs, Word docs, etc.)
  • Tests are added for new functionality and a local test run was successful
  • Appropriate documentation for new functionality has been added (Doxygen in the code and Markdown files if necessary)
  • Proper attention to licenses, especially no new third-party software with conflicting license has been added

Checks by code reviewer(s)

  • Corresponding issue(s) is/are linked and addressed
  • Code is clean of development artifacts (no deactivated or commented code lines, no debugging printouts, etc.)
  • Appropriate unit tests have been added, CI passes and code coverage is acceptable (did not decrease)
  • No large data files added in the whole history of commits(files should in sum not exceed 100 KB, avoid PDFs, Word docs, etc.)

Close #537

Sorry, something went wrong.

@dabele dabele requested a review from xsaschako October 13, 2023 16:28
@dabele dabele self-assigned this Oct 13, 2023
@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (aaacb23) 94.98% compared to head (1257aa9) 95.01%.
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #801      +/-   ##
==========================================
+ Coverage   94.98%   95.01%   +0.02%     
==========================================
  Files         112      112              
  Lines        8776     8800      +24     
==========================================
+ Hits         8336     8361      +25     
+ Misses        440      439       -1     

see 8 files with indirect coverage changes

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

Copy link
Member

@xsaschako xsaschako left a comment

Choose a reason for hiding this comment

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

nicemock makes sense here

Copy link
Member

@xsaschako xsaschako left a comment

Choose a reason for hiding this comment

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

nicemock makes sense here,although this leads to a function not being called in timeseries. This currently needs to be fixed for a merge

@dabele
Copy link
Member Author

dabele commented Oct 20, 2023

I added a basic test for the PrintTo function. I wouldn't want the test to in any way restrict what the output should be like or be difficult to maintain, so it basically just tests that the function exists and does something.

@dabele dabele requested a review from xsaschako October 20, 2023 09:23
Copy link
Member

@xsaschako xsaschako left a comment

Choose a reason for hiding this comment

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

Makes sense and looks good, thank you.

@xsaschako xsaschako merged commit e387504 into main Oct 20, 2023
@xsaschako xsaschako deleted the 537-remove-gmock-warnings branch October 20, 2023 10:12
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.

Gmock warnings in DynamicNPIs unittest
2 participants