Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/22713 )
Change subject: Gcovr support with coverage post-processing ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/22713/2/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: http://gerrit.cloudera.org:8080/#/c/22713/2/build-support/jenkins/build-and-test.sh@549 PS2, Line 549: .*/src/kudu/te > Here and below: is this about excluding everything under a directory (i.e. Yep that was the intent. Added more specific path patterns. http://gerrit.cloudera.org:8080/#/c/22713/2/build-support/jenkins/build-and-test.sh@552 PS2, Line 552: /src/kudu/i > is this a typo? yep, thanks for catching this! http://gerrit.cloudera.org:8080/#/c/22713/2/build-support/jenkins/build-and-test.sh@561 PS2, Line 561: FAILURES="$FAILURES"$'gcovr report processing failed\n' > Should the code below run only if gcovr invocation above succeeded? Yes that was the intent, changed to an if-else to not even create the venv etc if gcovr fails. http://gerrit.cloudera.org:8080/#/c/22713/2/build-support/jenkins/build-and-test.sh@566 PS2, Line 566: > nit: gcovr done. http://gerrit.cloudera.org:8080/#/c/22713/2/build-support/process_gcovr_report.py File build-support/process_gcovr_report.py: http://gerrit.cloudera.org:8080/#/c/22713/2/build-support/process_gcovr_report.py@40 PS2, Line 40: from bs4 import BeautifulSoup > Maybe add pip install bs4 to bootstrap-dev-env.sh or create a bootstrap-gco In the bootstrap-dev-env.sh we dont do any python package setup. In bootstrap-python-env.sh we basically setup the env for python client development. Adding bs4 there does not quite fit. On Jenkins we run build-and-test.sh where I've added a proper venv setup and teardown where bs4 is installed. Moreover if someone wants to run coverage locally the CSP and path limitations that are solved through this script should not be an issue to view the results locally. I dont quite expect developers to run full coverage locally (because that requires running all the tests :D). However on this note it might be useful to add some documentation on how to run coverage for a given file. As that is more likely if someone wants to bring up coverage for a given file. It might be worth to have the ability to do coverage % verification locally and quickly. Created a ticket for this: KUDU-3656. http://gerrit.cloudera.org:8080/#/c/22713/2/build-support/process_gcovr_report.py@45 PS2, Line 45: pr > Here and below: looking at scripts in src/kudu/scripts and other *.py files Ah totally disregarded this fact. Fixed it, thanks for pointing this out! http://gerrit.cloudera.org:8080/#/c/22713/2/build-support/process_gcovr_report.py@51 PS2, Line 51: rts_folder): : print(f"Error: '{reports_folder}' is not a director > Error: {reports_folder} is not an existing directory. It might exist as a f done. http://gerrit.cloudera.org:8080/#/c/22713/2/build-support/process_gcovr_report.py@69 PS2, Line 69: ed_h > could we rename this to something else? reading soup.findAll is a bit weird Makes sense, changed soup to parsed_html. Good readability catch. -- To view, visit http://gerrit.cloudera.org:8080/22713 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9d08cbb03234ec97cda985a35ac1b10a5d28ca20 Gerrit-Change-Number: 22713 Gerrit-PatchSet: 3 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Wed, 02 Apr 2025 11:03:28 +0000 Gerrit-HasComments: Yes
