Modernize `CondCore/ESSources/test` #36429

francescobrivio · 2021-12-09T09:15:15Z

PR description:

Following comment in #36419 (comment) I modernized scripts in CondCore/ESSources/test to:

use esConsumes
migrated legacy EDModules to be thread-safe
changed cout to LogPrint

PR validation:

Code compiles

Backport:

Not a backport, no backport needed.

CondCore/ESSources/test/SiStripTest/noiseanalyzer.cc

cmsbuild · 2021-12-09T09:27:09Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36429/27251

This PR adds an extra 16KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CondCore/ESSources/test/stubs/LumiTestReadAnalyzer.cc modified in PR(s): Apply Naming Rules to BeamSpotObjects methods #36419

cmsbuild · 2021-12-09T09:27:36Z

A new Pull Request was created by @francescobrivio for master.

It involves the following packages:

CondCore/ESSources (db, alca)

@malbouis, @yuanchao, @cmsbuild, @ggovi, @francescobrivio, @tvami can you please review it and eventually sign? Thanks.
@mmusich, @tocheng this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

mmusich

Thanks @francescobrivio, a couple of other comments.
Also I think there are more modules that could be made global (but not strong opinion, as these are test programs).

CondCore/ESSources/test/stubs/PedestalsAnalyzer.cc

CondCore/ESSources/test/stubs/TestUpdater.cc

cmsbuild · 2021-12-09T12:49:15Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36429/27260

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CondCore/ESSources/test/stubs/LumiTestReadAnalyzer.cc modified in PR(s): Apply Naming Rules to BeamSpotObjects methods #36419
- File CondCore/ESSources/test/stubs/TestConcurrentIOVsCondCore.cc modified in PR(s): Apply Naming Rules to BeamSpotObjects methods #36419

cmsbuild · 2021-12-09T12:49:42Z

Pull request #36429 was updated. @malbouis, @yuanchao, @cmsbuild, @ggovi, @francescobrivio, @tvami can you please check and sign again.

francescobrivio · 2021-12-09T13:36:34Z

@makortel in 49807ab I added edm::one::WatchRuns (as correctly pointed out in #36429 (review)) but:

in PedestalsAnalyzer.cc I had to also add explicitly endRun in order to compile succesfully
in TestUpdater.cc no endRun is required and the compilation is successful nonetheless

As far as I understood the code should always complain in this case when endRun in not added, could you help me understand what I am missing here?

cmsbuild · 2021-12-09T13:40:11Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36429/27263

This PR adds an extra 24KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CondCore/ESSources/test/stubs/LumiTestReadAnalyzer.cc modified in PR(s): Apply Naming Rules to BeamSpotObjects methods #36419
- File CondCore/ESSources/test/stubs/TestConcurrentIOVsCondCore.cc modified in PR(s): Apply Naming Rules to BeamSpotObjects methods #36419

cmsbuild · 2021-12-09T13:40:37Z

Pull request #36429 was updated. @malbouis, @yuanchao, @cmsbuild, @ggovi, @francescobrivio, @tvami can you please check and sign again.

mmusich · 2021-12-09T14:08:05Z

@francescobrivio

in 49807ab I added edm::one::WatchRuns (as correctly pointed out in suggested in ) but:
in `PedestalsAnalyzer.cc` I had to also add explicitly endRun in order to compile succesfully
in `TestUpdater.cc` no endRun is required and the compilation is successful nonetheless
As far as I understood the code should always complain in this case when endRun in not added, could you help me understand what I am missing here?

as far as I can see, that module is not even compiled:

cmssw/CondCore/ESSources/test/BuildFile.xml

Lines 1 to 33 in 9d1098c

    
           <use name="FWCore/ParameterSet"/> 
        
           <use name="CondFormats/Calibration"/> 
        
           <use name="FWCore/Framework"/> 
        
           <use name="CondFormats/DataRecord"/> 
        
           <use name="CondFormats/BeamSpotObjects"/> 
        
           <library file="stubs/PedestalsAnalyzer.cc" name="PedestalsAnalyzer"> 
        
             <flags EDM_PLUGIN="1"/> 
        
           </library> 
        
           <library file="stubs/PedestalsByLabelAnalyzer.cc" name="PedestalsByLabelAnalyzer"> 
        
             <flags EDM_PLUGIN="1"/> 
        
           </library> 
        
           <library file="SiStripTest/noiseanalyzer.cc" name="NoisesAnalyzer"> 
        
             <flags EDM_PLUGIN="1"/> 
        
           </library> 
        
           <library file="stubs/EfficiencyByLabelAnalyzer.cc" name="ExEfficiencyByLabelAnalyzer"> 
        
             <flags EDM_PLUGIN="1"/> 
        
           </library> 
        
           <library file="stubs/TestConcurrentIOVsCondCore.cc" name="TestConcurrentIOVsCondCore"> 
        
             <flags EDM_PLUGIN="1"/> 
        
           </library> 
        
           <bin file="TestCondCoreESSources.cpp" name="UnitTestConcurrentIOVsCondCore"> 
        
             <flags TEST_RUNNER_ARGS=" /bin/bash CondCore/ESSources/test TestConcurrentIOVsCondCore.sh"/> 
        
             <use name="FWCore/Utilities"/> 
        
           </bin> 
        
           <library file="stubs/LumiTestReadAnalyzer.cc" name="LumiTestReadAnalyzer"> 
        
             <flags EDM_PLUGIN="1"/> 
        
           </library>

CondCore/ESSources/test/stubs/PedestalsAnalyzer.cc

francescobrivio · 2021-12-09T23:21:26Z

@cmsbuild please test

cmsbuild · 2021-12-09T23:25:17Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36429/27282

This PR adds an extra 16KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CondCore/ESSources/test/stubs/LumiTestReadAnalyzer.cc modified in PR(s): Apply Naming Rules to BeamSpotObjects methods #36419
- File CondCore/ESSources/test/stubs/TestConcurrentIOVsCondCore.cc modified in PR(s): Apply Naming Rules to BeamSpotObjects methods #36419

cmsbuild · 2021-12-09T23:25:41Z

Pull request #36429 was updated. @ggovi, @malbouis, @tvami, @yuanchao, @francescobrivio can you please check and sign again.

cmsbuild · 2021-12-10T06:41:54Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d5d596/21150/summary.html
COMMIT: 2159f8d
CMSSW: CMSSW_12_3_X_2021-12-09-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36429/21150/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 3250704
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3250682
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

francescobrivio · 2021-12-10T09:59:45Z

@tvami @malbouis @ggovi this is now passing all tests nicely, please review/sign at your convenience

ggovi · 2021-12-10T20:22:36Z

CondCore/ESSources/test/SiStripTest/noiseanalyzer.cc


  private:
+    const edm::ESGetToken<mySiStripNoises, mySiStripNoisesRcd> theNoisesToken_;


@francescobrivio does this token ( and all the others added ) need to be initialise with esConsumes ?

it's initialized few lines before (line 19):

explicit NoisesAnalyzer(edm::ParameterSet const& p) : theNoisesToken_(esConsumes())

@francescobrivio thanks, did not noticed...

tvami · 2021-12-10T22:20:44Z

+alca

tvami · 2021-12-12T15:17:22Z

+db

seems like @ggovi agreed and anyway everything is in test

cmsbuild · 2021-12-12T15:17:46Z

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 will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

qliphy · 2021-12-13T01:12:14Z

+1

use esConsumes in CondCore/ESSources/test

da04738

cmsbuild added this to the CMSSW_12_3_X milestone Dec 9, 2021

cmsbuild added alca-pending code-checks-pending db-pending orp-pending pending-signatures tests-pending labels Dec 9, 2021

francescobrivio mentioned this pull request Dec 9, 2021

Apply Naming Rules to BeamSpotObjects methods #36419

Merged

mmusich reviewed Dec 9, 2021

View reviewed changes

CondCore/ESSources/test/SiStripTest/noiseanalyzer.cc Outdated Show resolved Hide resolved

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 9, 2021

cmsbuild added code-checks-pending and removed code-checks-approved labels Dec 9, 2021

mmusich reviewed Dec 9, 2021

View reviewed changes

CondCore/ESSources/test/stubs/PedestalsAnalyzer.cc Outdated Show resolved Hide resolved

CondCore/ESSources/test/stubs/TestUpdater.cc Outdated Show resolved Hide resolved

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 9, 2021

cmsbuild added code-checks-pending and removed code-checks-approved labels Dec 9, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 9, 2021

mmusich reviewed Dec 9, 2021

View reviewed changes

CondCore/ESSources/test/stubs/PedestalsAnalyzer.cc Outdated Show resolved Hide resolved

CondCore/ESSources/test/stubs/PedestalsAnalyzer.cc Outdated Show resolved Hide resolved

cmsbuild added the tests-pending label Dec 9, 2021

cmsbuild added tests-started and removed tests-pending labels Dec 9, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 9, 2021

cmsbuild added tests-approved and removed tests-started labels Dec 10, 2021

ggovi reviewed Dec 10, 2021

View reviewed changes

cmsbuild added alca-approved and removed alca-pending labels Dec 10, 2021

cmsbuild added db-approved fully-signed and removed db-pending pending-signatures labels Dec 12, 2021

cmsbuild added orp-approved and removed orp-pending labels Dec 13, 2021

cmsbuild merged commit 61e8b00 into cms-sw:master Dec 13, 2021

francescobrivio deleted the alca-esConsumesCondCoreESSources branch December 13, 2021 15:16

This was referenced Dec 13, 2021

[12.3.X] Move to root 6.24 cms-sw/cmsdist#7500

Merged

[GCC11] Fix warning "strncmp ... evaluates to nonzero" #36467

Merged

[ROOT624] Testing root 62.4 for gcc10 cms-sw/cmsdist#7505

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize `CondCore/ESSources/test` #36429

Modernize `CondCore/ESSources/test` #36429

francescobrivio commented Dec 9, 2021 •

edited

Loading

cmsbuild commented Dec 9, 2021

cmsbuild commented Dec 9, 2021

mmusich left a comment

cmsbuild commented Dec 9, 2021

cmsbuild commented Dec 9, 2021

francescobrivio commented Dec 9, 2021 •

edited

Loading

cmsbuild commented Dec 9, 2021

cmsbuild commented Dec 9, 2021

mmusich commented Dec 9, 2021 •

edited

Loading

francescobrivio commented Dec 9, 2021

cmsbuild commented Dec 9, 2021

cmsbuild commented Dec 9, 2021

cmsbuild commented Dec 10, 2021

francescobrivio commented Dec 10, 2021

ggovi Dec 10, 2021

francescobrivio Dec 10, 2021

ggovi Dec 11, 2021

tvami commented Dec 10, 2021

tvami commented Dec 12, 2021

cmsbuild commented Dec 12, 2021

qliphy commented Dec 13, 2021


		private:
		const edm::ESGetToken<mySiStripNoises, mySiStripNoisesRcd> theNoisesToken_;

Modernize CondCore/ESSources/test #36429

Modernize CondCore/ESSources/test #36429

Conversation

francescobrivio commented Dec 9, 2021 • edited Loading

PR description:

PR validation:

Backport:

cmsbuild commented Dec 9, 2021

cmsbuild commented Dec 9, 2021

mmusich left a comment

Choose a reason for hiding this comment

cmsbuild commented Dec 9, 2021

cmsbuild commented Dec 9, 2021

francescobrivio commented Dec 9, 2021 • edited Loading

cmsbuild commented Dec 9, 2021

cmsbuild commented Dec 9, 2021

mmusich commented Dec 9, 2021 • edited Loading

francescobrivio commented Dec 9, 2021

cmsbuild commented Dec 9, 2021

cmsbuild commented Dec 9, 2021

cmsbuild commented Dec 10, 2021

Comparison Summary

francescobrivio commented Dec 10, 2021

ggovi Dec 10, 2021

Choose a reason for hiding this comment

francescobrivio Dec 10, 2021

Choose a reason for hiding this comment

ggovi Dec 11, 2021

Choose a reason for hiding this comment

tvami commented Dec 10, 2021

tvami commented Dec 12, 2021

cmsbuild commented Dec 12, 2021

qliphy commented Dec 13, 2021

Modernize `CondCore/ESSources/test` #36429

Modernize `CondCore/ESSources/test` #36429

francescobrivio commented Dec 9, 2021 •

edited

Loading

francescobrivio commented Dec 9, 2021 •

edited

Loading

mmusich commented Dec 9, 2021 •

edited

Loading