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

Consumes migration for triggerResultsByName #14262

Merged
merged 1 commit into from Apr 27, 2016

Conversation

wddgit
Copy link
Contributor

@wddgit wddgit commented Apr 26, 2016

Note that SUSYBSMAnalysis/HSCP/plugins/HSCPValidator.cc shows many lines changed only because the previous version had DOS line endings. Ignoring that, the changes in that file are almost the same as in SUSYBSMAnalysis/HSCP/plugins/HSCPHLTFilter.cc.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @wddgit (W. David Dagenhart) for CMSSW_8_1_X.

It involves the following packages:

DataFormats/FWLite
FWCore/Common
FWCore/Framework
PhysicsTools/FWLite
SUSYBSMAnalysis/HSCP

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

cms-bot commands are list here #13028

@Dr15Jones
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 26, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/12655/console

if(resultsByNameHLT.isValid() && expectedTriggerResultsHLT_.size() > 0) {
edm::InputTag tag("TriggerResults", "", "HLT");
edm::Handle<edm::TriggerResults> hTriggerResults;
e.getByLabel(tag, hTriggerResults);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is Core code, how about changing to getByToken?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This a module used only for tests. It has both a call to getManyByType and also getByLabel, both getting the TriggerResults type. There is a consumesMany call that allows both of those to work. I am not sure what happens if you call both consumes to get a token and also consumesMany for the same type.

@Dr15Jones
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

-1

Tested at: 140e01a

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
25fce4e
e44a862
a11f326
5fb9f4b
b3db0a9
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-14262/12655/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-14262/12655/git-merge-result

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-14262/12655/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:
25fce4e
e44a862
a11f326
5fb9f4b
b3db0a9
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-14262/12655/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-14262/12655/git-merge-result

@cmsbuild
Copy link
Contributor

@davidlange6
Copy link
Contributor

@smuzaffar - it seems that the error harvesting didn't work above...

@davidlange6 davidlange6 merged commit e2edf29 into cms-sw:CMSSW_8_1_X Apr 27, 2016
@Dr15Jones
Copy link
Contributor

@smuzaffar Looking at the unit test log I found

===== Test "TestSiStripDCS" ====
TestSiStripDCS::testEquality. : OK
TestSiStripDCS::testAddition. : OK
TestSiStripDetVOffBuilder::testConstructor. : errorE
TestSiStripDetVOffBuilder::testReduction. : errorE



!!!FAILURES!!!
Test Results:
Run:  4   Failures: 0   Errors: 2


1) test: TestSiStripDetVOffBuilder::testConstructor (E) 
setUp() failed
- uncaught exception of type N3edm9ExceptionE
- An exception of category 'Configuration' occurred.
Exception Message:
MissingParameter: Parameter 'DeltaTmin' not found.



2) test: TestSiStripDetVOffBuilder::testReduction (E) 
setUp() failed
- uncaught exception of type N3edm9ExceptionE
- An exception of category 'Configuration' occurred.
Exception Message:
MissingParameter: Parameter 'DeltaTmin' not found.




---> test TestSiStripDCS succeeded

Very strange that cppunit said there was an error but the test framework thought there was not.

@Dr15Jones
Copy link
Contributor

I think I found the reason scram thinks everything is OK. The test writer didn't use the standard way of running the tests:
http://cmslxr.fnal.gov/source/CalibTracker/SiStripDCS/test/UnitTests/MasterTestSiStripDCS.cpp?v=CMSSW_8_1_X_2016-04-25-2300

that specially made main always returns 0 so scram never gets to know there was a problem.

@Dr15Jones
Copy link
Contributor

#14270

@smuzaffar
Copy link
Contributor

@davidlange6 , it could be that unit tests took more than 7200s to finish.
@Dr15Jones, scram only uses the exit code of the tests. For this test TestSiStripDCS it is returning 0.

@wddgit wddgit deleted the consumesTriggerResultsByName branch September 23, 2016 14:03
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