Skip to content

(Re)introduce a list of macros that can avoid testing - #2188

Merged
shahor02 merged 1 commit into
AliceO2Group:devfrom
aphecetche:macro-test-exclusion-list
Jul 11, 2019
Merged

(Re)introduce a list of macros that can avoid testing#2188
shahor02 merged 1 commit into
AliceO2Group:devfrom
aphecetche:macro-test-exclusion-list

Conversation

@aphecetche

Copy link
Copy Markdown
Collaborator

@shahor02 @davidrohr would that work for you ?

btw, in the list of excluded macros, are there some that you know should actually be tested ? (I must confess I'm not so sure for all of them why they ended up in the non-tested list...)

@aphecetche
aphecetche force-pushed the macro-test-exclusion-list branch from 58d9e1d to bfce748 Compare July 10, 2019 21:27
@shahor02

Copy link
Copy Markdown
Collaborator

@aphecetche yes, this logics looks fine for me, but I don't understand failing checkcode, it mentions Detectors/gconfig/commonConfig.C as excluded (while it is not and its test is requested) then produces a Fatal on it.

Concerning currently excluded macros:

Detectors/FIT/T0/macros/run_reco_t0.C
Detectors/FIT/T0/macros/readHitsDigits.C
Detectors/ITSMFT/ITS/macros/test/CheckLookUp.C

are operational macros (I used them occasionally) and passing the test, why they were excluded?

@davidrohr

davidrohr commented Jul 11, 2019 via email

Copy link
Copy Markdown
Collaborator

@aphecetche

Copy link
Copy Markdown
Collaborator Author

@shahor02 the o2checkcode fails because some macros require simulation parts that are not built. Means I have to change a bit the logic : always call o2_add_test_root_macro but add a parameter to signal whether or not it should actually perform the test, depending on whether or not BUILD_SIMULATION is on.

For the 3 missing macros, no particular reason as far as I can tell, will put them back.

@davidrohr

davidrohr commented Jul 11, 2019 via email

Copy link
Copy Markdown
Collaborator

@shahor02

Copy link
Copy Markdown
Collaborator

@aphecetche I refer to this failure in checkode:

-- [ ] [ ] [E] Detectors/gconfig/commonConfig.C
CMake Error at cmake/O2ReportNonTestedMacros.cmake:79 (message):
  Macro Detectors/gconfig/commonConfig.C should be tested
Call Stack (most recent call first):
  CMakeLists.txt:112 (o2_report_non_tested_macros)

macro declared as excluded and then reported to be in neither of the list (while it is requested to be tested)

@aphecetche

Copy link
Copy Markdown
Collaborator Author

@shahor02 o2_add_test_root_macro for commonConfig.C is called in a sub_directory which is not included when BUILD_SIMULATION=OFF (which is the case when some pythia or geant stuff is not found, e.g. on o2checkcode CI slaves)

Contrary to my comment above though, can't really add a parameter to o2_add_test_root_macro as in the case for commonConfig.C exemplifies, sometimes a full CMakeLists.txt is skipped.

Guess the exclusion list have to include a couple of if(BUILD_SIMULATION) and if(pythia6_FOUND) ...

@shahor02

Copy link
Copy Markdown
Collaborator

I see, thanks!

@aphecetche
aphecetche force-pushed the macro-test-exclusion-list branch from bfce748 to 18cf2fa Compare July 11, 2019 08:42
@aphecetche

Copy link
Copy Markdown
Collaborator Author

on closer look :

Detectors/FIT/T0/macros/run_reco_t0.C
Detectors/FIT/T0/macros/readHitsDigits.C

were excluded because they are the same as the ones in Detectors/FIT/macros so in the end I've simply removed those duplicates.

For the rest, macros that are specific to simulation are now put on the exclusion list when required. That should fix the o2checkcode build.

@aphecetche aphecetche changed the title [WIP] (Re)introduce a list of macros that can avoid testing (Re)introduce a list of macros that can avoid testing Jul 11, 2019
@aphecetche

Copy link
Copy Markdown
Collaborator Author

@shahor02 @davidrohr Don't exactly understand the issue with clang-format ...

Doing git-clang-format locally the line reported by travis I get :

> git-clang-format --commit 347200c0716c5d2e0467af7b7946b9f5b58790a2 --diff
no modified files to format

..... but the others are green.

@shahor02

Copy link
Copy Markdown
Collaborator

@aphecetche It complains about the macro which should be anyway removed since duplicated (as you found). I'll merge this PR then remove the macro and update the lists

@shahor02
shahor02 merged commit 4b3129a into AliceO2Group:dev Jul 11, 2019
EmilGorm pushed a commit to EmilGorm/AliceO2 that referenced this pull request Apr 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants