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

IOMC/RandomEngine/test fix bug in initialization of sleep timer. #17962

Merged
merged 2 commits into from Mar 18, 2017
Merged

IOMC/RandomEngine/test fix bug in initialization of sleep timer. #17962

merged 2 commits into from Mar 18, 2017

Conversation

gartung
Copy link
Member

@gartung gartung commented Mar 16, 2017

This is a bug because the intent was to sleep 0.025 seconds not 0 seconds.

/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/f60d4bf7e099ce1e597b02cf09f0745b/opt/cmssw/slc6_amd64_gcc530/cms/cmssw-patch/CMSSW_9_1_CLANG_X_2017-03-15-2300/src/IOMC/RandomEngine/test/TestRandomNumberServiceGlobal.cc:191:11: warning: implicit conversion from 'double' to 'unsigned int' changes value from 0.025 to 0 [-Wliteral-conversion]
sleep(0.025);
~~~~~ ^~~~~

…' changes value from 0.025 to 0 [-Wliteral-conversion]

This is a bug because the intent was to sleep 0.025 seconds not 0 seconds.

  /build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/f60d4bf7e099ce1e597b02cf09f0745b/opt/cmssw/slc6_amd64_gcc530/cms/cmssw-patch/CMSSW_9_1_CLANG_X_2017-03-15-2300/src/IOMC/RandomEngine/test/TestRandomNumberServiceGlobal.cc:191:11: warning: implicit conversion from 'double' to 'unsigned int' changes value from 0.025 to 0 [-Wliteral-conversion]
     sleep(0.025);
    ~~~~~ ^~~~~
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @gartung (Patrick Gartung) for master.

It involves the following packages:

IOMC/RandomEngine

@cmsbuild, @smuzaffar, @Dr15Jones, @davidlange6 can you please review it and eventually sign? Thanks.
@wddgit, @wmtan this is something you requested to watch as well.
@Muzaffar, @davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@wddgit
Copy link
Contributor

wddgit commented Mar 16, 2017

I could be wrong, but I think this fix will still round to 0 even if it evades the warning because the argument of sleep is an unsigned int. I am not sure what the best alternative is. There is nanosleep and also std::this_thread::sleep_for, although I have not actually ever used either one and do not know if they are appropriate for this situation. The second one is in and I do not know whether that is OK or not.

@gartung
Copy link
Member Author

gartung commented Mar 16, 2017

How about usleep(25000)?

@cmsbuild
Copy link
Contributor

Pull request #17962 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @davidlange6 can you please check and sign again.

@wddgit
Copy link
Contributor

wddgit commented Mar 16, 2017

Looks good to me. I looked and usleep is used other places in CMSSW.

@Dr15Jones
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 16, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18503/console Started: 2017/03/16 22:17

@cmsbuild
Copy link
Contributor

-1

Tested at: dd01668

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
1176782
d4ce470
4af5d48
31b2a25
e21d662
56f3a99
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17962/18503/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17962/18503/git-merge-result

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17962/18503/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

  • Unit Tests:

I found errors in the following unit tests:

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
1176782
d4ce470
4af5d48
31b2a25
e21d662
56f3a99
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17962/18503/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17962/18503/git-merge-result

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@Dr15Jones
Copy link
Contributor

Please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 17, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18515/console Started: 2017/03/17 16:55

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@Dr15Jones
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @Muzaffar, @davidlange6, @smuzaffar

@cmsbuild
Copy link
Contributor

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit acfe935 into cms-sw:master Mar 18, 2017
@gartung gartung deleted the IOMC_RandomEngine_test_fix_clang_warning branch April 14, 2017 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants