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

iox-#692 Move tsan jobs into build-test.yml #1888

Conversation

ibrhmkuru
Copy link
Contributor

@ibrhmkuru ibrhmkuru commented Feb 9, 2023

Signed-off-by: Ibrahim Kuru [email protected]

Pre-Review Checklist for the PR Author

  1. Code follows the coding style of CONTRIBUTING.md
  2. Tests follow the best practice for testing
  3. Changelog updated in the unreleased section including API breaking changes
  4. Branch follows the naming format (iox-123-this-is-a-branch)
  5. Commits messages are according to this guideline
  6. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  7. Relevant issues are linked
  8. Add sensible notes for the reviewer
  9. All checks have passed (except task-list-completed)
  10. All touched (C/C++) source code files from iceoryx_hoofs are added to ./clang-tidy-diff-scans.txt
  11. Assign PR to reviewer

Notes for Reviewer

  • build-test-ubuntu-with-thread-sanitizer-clang-latest job moved from Code Linting on master branch workflow into Build & Test workflow. So, thread sanitizer will work for every pull request.
  • Suppression file refined. For mutex and deadlock type of warnings, only the problematic functions are suppressed.

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

@ibrhmkuru ibrhmkuru marked this pull request as ready for review February 9, 2023 10:38
@ibrhmkuru ibrhmkuru force-pushed the iox-692-move-thread-sanitizer-job-to-pr-ci branch 2 times, most recently from e0f4cc6 to d239fbc Compare February 9, 2023 12:13
@dkroenke
Copy link
Member

dkroenke commented Feb 9, 2023

This needs to wait until #1879 got merged to re-evaluate the scan time.

@elBoberido
Copy link
Member

@ibrhmkuru It seems the tsan job now finishes in about 23 minutes. https://github.com/eclipse-iceoryx/iceoryx/actions/runs/4165393238/jobs/7208329322. I think we do not need to split the test runs up.

cc @dkroenke

@mossmaurice mossmaurice added the tooling All iceoryx related tooling (scripts etc.) label Feb 15, 2023
@ibrhmkuru ibrhmkuru force-pushed the iox-692-move-thread-sanitizer-job-to-pr-ci branch from c6aaa31 to 25c89f6 Compare February 16, 2023 10:43
@dkroenke dkroenke self-requested a review February 16, 2023 11:27
@dkroenke
Copy link
Member

@ibrhmkuru The job itself looks good and the execution time is acceptable, we can bring this into master.

There are only some formal things missing:

  • rebase the branch onto latest master and drop merge commit d58e0d2 and resolve merge conflicts
  • squash remaining commits into one
  • the commits and the Pull-Request needs to be linked to issue ThreadSanitizer for iceoryx #692
  • the commits need to be signed off
  • Fill out the Pull-Request Checklist

@ibrhmkuru ibrhmkuru force-pushed the iox-692-move-thread-sanitizer-job-to-pr-ci branch from 25c89f6 to b0eeac5 Compare February 16, 2023 12:21
@ibrhmkuru ibrhmkuru changed the title Move tsan jobs into build-test.yml iox-#692 Move tsan jobs into build-test.yml Feb 16, 2023
@ibrhmkuru ibrhmkuru closed this Feb 16, 2023
@ibrhmkuru ibrhmkuru force-pushed the iox-692-move-thread-sanitizer-job-to-pr-ci branch from b0eeac5 to 2f87030 Compare February 16, 2023 12:47
@dkroenke dkroenke reopened this Feb 16, 2023
@dkroenke dkroenke requested a review from elBoberido February 17, 2023 17:01
dkroenke
dkroenke previously approved these changes Feb 17, 2023
Copy link
Member

@dkroenke dkroenke left a comment

Choose a reason for hiding this comment

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

Scan time looks ok with ~25 minutes.

The error in the integrationtest is caused by another issue and will be fixed with #1907

LGTM!

@ibrhmkuru ibrhmkuru force-pushed the iox-692-move-thread-sanitizer-job-to-pr-ci branch 5 times, most recently from 57d2e96 to b169e18 Compare February 17, 2023 22:48
@ibrhmkuru ibrhmkuru force-pushed the iox-692-move-thread-sanitizer-job-to-pr-ci branch from b169e18 to 002c083 Compare February 17, 2023 23:36
@codecov
Copy link

codecov bot commented Feb 18, 2023

Codecov Report

Merging #1888 (002c083) into master (2f87030) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1888      +/-   ##
==========================================
+ Coverage   75.44%   75.45%   +0.01%     
==========================================
  Files         383      383              
  Lines       15181    15181              
  Branches     2148     2148              
==========================================
+ Hits        11453    11455       +2     
  Misses       3055     3055              
+ Partials      673      671       -2     
Flag Coverage Δ
unittests 75.11% <ø> (+0.01%) ⬆️
unittests_timing 15.43% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
iceoryx_hoofs/source/concurrent/loffli.cpp 87.87% <0.00%> (+6.06%) ⬆️

@elBoberido elBoberido requested a review from dkroenke February 20, 2023 17:43
@ibrhmkuru
Copy link
Contributor Author

@dkroenke Can you approve if changes are okay ?

@ibrhmkuru
Copy link
Contributor Author

@dkroenke Do you have any additional review comments?

Copy link
Member

@dkroenke dkroenke left a comment

Choose a reason for hiding this comment

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

LGTM

@dkroenke dkroenke merged commit d51180d into eclipse-iceoryx:master Feb 27, 2023
@dkroenke dkroenke deleted the iox-692-move-thread-sanitizer-job-to-pr-ci branch February 27, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling All iceoryx related tooling (scripts etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants