Programming exercises: Enable static code analysis for C++ with LocalCI
#10453
Conversation
github-actions
bot
added
tests
server
magaupp
force-pushed
the
feature/programming-exercises/cplusplus-sca
branch
from
fe61bf5 to
45234d2
Compare
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
WalkthroughThis pull request introduces a new constructor in the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant M as Main (default_static.sh)
participant S as setup_the_build_environment
participant T as build_and_run_all_tests
participant A as static_code_analysis
participant F as final_aeolus_post_action
U->>M: Execute script
M->>S: Set up build environment
S-->>M: Environment ready
M->>T: Run tests
T-->>M: Test results
M->>A: Run Clang-Tidy analysis
A-->>M: Analysis report generated
M->>F: Execute final post actions
F-->>M: Post actions complete
sequenceDiagram
participant C as Caller
participant PP as ParserPolicy
participant SP as SarifParser
participant CC as SingleCategoryCategorizer
C->>PP: configure(fileName)
alt file indicates CLANG_TIDY
PP->>SP: Create new SarifParser(CLANG_TIDY, CC)
SP-->>PP: Parser instance with Clang-Tidy support
else other cases
PP-->>C: Return appropriate parser
end
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/main/resources/templates/c_plus_plus/staticCodeAnalysis/test/Tests.py (2)
1-34: Test script follows good practices but lacks error handlingThe script is well-structured and correctly imports necessary components, configures the build environment, and runs tests. However, there's no error handling in case of failures during configuration or test execution.
Consider adding basic error handling to gracefully handle failures:
# Run the actual tests: -tester.run() +try: + tester.run() +except Exception as e: + print(f"Error running tests: {e}") + import sys + sys.exit(1) # Export the results into the JUnit XML format: -tester.exportResult("./test-reports/tests-results.xml") +try: + tester.exportResult("./test-reports/tests-results.xml") +except Exception as e: + print(f"Error exporting test results: {e}") + import sys + sys.exit(1)
13-13: Consider making the build directory configurableThe build directory is hardcoded as "./build", which limits flexibility.
Consider making it configurable through command-line arguments:
+import argparse + def main() -> None: + parser = argparse.ArgumentParser(description='Run C++ tests with static analysis') + parser.add_argument('--build-dir', default='./build', help='Build directory') + args = parser.parse_args() + # Create a new instance of the tester: tester: Tester = Tester() - buildDir = "./build" + buildDir = args.build_dirsrc/main/resources/templates/aeolus/c_plus_plus/default_static.sh (1)
1-79: Script follows good practices but could benefit from input validationThe script is well-structured with clear function separations and proper error handling. Consider adding validation for essential environment variables and commands to fail gracefully with informative error messages.
#!/usr/bin/env bash set -e export AEOLUS_INITIAL_DIRECTORY=${PWD} + # Validate required tools are available + command -v clang-tidy >/dev/null 2>&1 || { echo "Error: clang-tidy is required but not installed."; exit 1; } + command -v clang-tidy-sarif >/dev/null 2>&1 || { echo "Error: clang-tidy-sarif converter is required but not installed."; exit 1; } +🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 54-54: studentParentWorkingDirectoryName is referenced but not assigned.
(SC2154)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/main/resources/config/application.ymlis excluded by!**/*.ymlsrc/main/resources/templates/aeolus/c_plus_plus/default_static.yamlis excluded by!**/*.yamlsrc/main/resources/config/application.ymlis excluded by!**/*.yml
📒 Files selected for processing (14)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/format/sarif/ReportingDescriptor.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/sarif/SarifParser.java(1 hunks)docs/user/exercises/programming-exercise-features.inc(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/StaticCodeAnalysisConfigurer.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/domain/StaticCodeAnalysisTool.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/ParserPolicy.java(1 hunks)src/main/resources/templates/aeolus/c_plus_plus/default_static.sh(1 hunks)src/main/resources/templates/c_plus_plus/staticCodeAnalysis/test/.clang-tidy(1 hunks)src/main/resources/templates/c_plus_plus/staticCodeAnalysis/test/Tests.py(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/StaticCodeAnalysisParserUnitTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java(1 hunks)src/test/resources/test-data/static-code-analysis/expected/clang-tidy.json(1 hunks)src/test/resources/test-data/static-code-analysis/reports/clang-tidy.sarif(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/programming/StaticCodeAnalysisParserUnitTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/config/StaticCodeAnalysisConfigurer.javasrc/main/java/de/tum/cit/aet/artemis/programming/domain/StaticCodeAnalysisTool.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/ParserPolicy.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/sarif/SarifParser.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/format/sarif/ReportingDescriptor.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java
🧠 Learnings (2)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/sarif/SarifParser.java (1)
Learnt from: magaupp
PR: ls1intum/Artemis#10204
File: src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/sarif/RubocopCategorizer.java:13-15
Timestamp: 2025-01-28T17:50:58.545Z
Learning: In the Artemis codebase, null checks for ReportingDescriptor and rule.id() are handled during SARIF parsing before reaching the categorizer classes.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java (1)
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java:43-55
Timestamp: 2024-11-12T12:51:40.391Z
Learning: For constructors with multiple boolean parameters, it's acceptable to keep them as is because parameter names are clear and IDEs provide inline hints, making the code readable without refactoring to the builder pattern.
🪛 Shellcheck (0.10.0)
src/main/resources/templates/aeolus/c_plus_plus/default_static.sh
[warning] 54-54: studentParentWorkingDirectoryName is referenced but not assigned.
(SC2154)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (15)
src/main/resources/templates/c_plus_plus/staticCodeAnalysis/test/.clang-tidy (1)
1-2: LGTM! Configuration enables appropriate Clang-Tidy checksThe configuration correctly enables all diagnostic and analyzer checks through wildcards, which is a good starting point for static code analysis in C++.
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java (1)
356-356: LGTM! Proper integration of CLANG_TIDY in the switch statementThe addition of the CLANG_TIDY case with "Lint" as the category aligns well with the existing pattern used for other linting tools.
src/test/java/de/tum/cit/aet/artemis/programming/StaticCodeAnalysisParserUnitTest.java (1)
96-99: LGTM! Well-structured test method for Clang-Tidy parserThe test follows the established pattern in the class, has appropriate JUnit5 annotation, and correctly references the input SARIF file and expected JSON output.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/format/sarif/ReportingDescriptor.java (1)
35-42: LGTM! Well-documented convenience constructorThe new constructor provides a clean way to instantiate a ReportingDescriptor with just an ID, which is useful when other fields aren't needed. The JavaDoc clearly explains its purpose.
src/test/resources/test-data/static-code-analysis/expected/clang-tidy.json (1)
1-16: Correctly structured test fixture for Clang-Tidy outputThe JSON structure looks well-formed and provides a good test fixture for validating the Clang-Tidy analyzer integration. It correctly includes the essential fields needed for static analysis reporting: tool identification, file path, line/column position, rule ID, category, and priority level.
docs/user/exercises/programming-exercise-features.inc (2)
70-70: Documentation updated to reflect Python static code analysis supportThe feature table has been properly updated to indicate that static code analysis for Python is now supported in Local CI but not in Jenkins.
94-94: Documentation updated to reflect C++ static code analysis supportThe feature table has been correctly updated to indicate that static code analysis for C++ is now supported in Local CI but not in Jenkins, which aligns with the PR objectives.
src/test/resources/test-data/static-code-analysis/reports/clang-tidy.sarif (1)
1-34: Well-formatted SARIF report for Clang-TidyThe SARIF file structure is correct and follows the standard 2.1.0 format. It properly includes all required elements: the tool driver information, result level, location details, and rule ID. This format will enable proper interoperability with other static analysis tools and platforms.
src/main/resources/templates/c_plus_plus/staticCodeAnalysis/test/Tests.py (1)
17-22: CMake configuration correctly includes compile commands exportThe
-DCMAKE_EXPORT_COMPILE_COMMANDS=ONflag is correctly included, which is essential for Clang-Tidy to function properly as it needs the compilation database to understand how files are compiled.src/main/java/de/tum/cit/aet/artemis/core/config/StaticCodeAnalysisConfigurer.java (1)
96-96: Appropriate addition of Clang-Tidy for C++ static code analysis.The C++ programming language has been properly configured with the Clang-Tidy tool using the generic lint category. This aligns well with the PR objective of enabling static code analysis for C++ using Clang-Tidy.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/ParserPolicy.java (1)
39-39: Good implementation of Clang-Tidy parser.The CLANG_TIDY case is implemented correctly in the switch statement, creating a SarifParser with the appropriate tool and categorizer. This follows the established pattern used for other single-category tools like ESLINT.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/sarif/SarifParser.java (1)
129-129: Improved rule categorization logic.The code has been simplified to use a consistent approach for rule categorization. Rather than using a separate fallback mechanism, the code now directly creates a new ReportingDescriptor from the rule ID when necessary. This simplification makes the code more maintainable while ensuring consistent behavior.
This implementation aligns with the learning from PR #10204 that null checks for ReportingDescriptor are handled during SARIF parsing.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java (1)
65-65: Enabled static code analysis for C++.The second parameter in the ProgrammingLanguageFeature constructor has been changed from false to true, enabling static code analysis support for C++. This change is essential for the PR objective of implementing Clang-Tidy analysis for C++.
src/main/java/de/tum/cit/aet/artemis/programming/domain/StaticCodeAnalysisTool.java (2)
17-17: New static code analysis tool for C++ looks good!The addition of CLANG_TIDY follows the existing pattern for tool declarations with the correct SARIF filename format.
39-39: C++ language mapping correctly associated with Clang-Tidy toolThis addition properly extends the static code analysis capabilities to support C++ programming exercises, aligning with the PR objective.
| @@ -67,7 +67,7 @@ Instructors can still use those templates to generate programming exercises and | |||
| +======================+======================+======================+=====================+==============+==========================================+==============================+========================+ | |||
| | Java | yes | yes | yes | yes | Gradle, Maven, J: `DejaGnu`_ | no | L: yes, J: no | | |||
| +----------------------+----------------------+----------------------+---------------------+--------------+------------------------------------------+------------------------------+------------------------+ | |||
| | Python | L: yes; J: no | no | yes | no | n/a | no | L: yes, J: no | | |||
| | Python | L: yes; J: no | L: yes; J: no | yes | no | n/a | no | L: yes, J: no | | |||
There was a problem hiding this comment.
Was this just not updated when it should have been, so you did it now?
There was a problem hiding this comment.
Yes, this was implemented a while ago in #9573
Checklist
General
Server
Changes affecting Programming Exercises
Motivation and Context
This PR enables static code analysis for C++ using Clang-Tidy.
Clang-Tidy can be configured using the
.clang-tidyfile in the test repository.Description
Clang-Tidy's output is converted to SARIF using a fork of sarif-rs. The fork moves the clang-tidy check name from the
messageto the SARIFruleId.The Docker image is updated to install
clang-tidyand our fork of theclang-tidy-sariftool.ls1intum/artemis-cpp-docker@0115918
The SARIF parser is updated to use the provided RuleCategorizer with a dummy ReportingDescriptor if the SARIF file does not contain any.
The Python test script is updated to configure CMake with
CMAKE_EXPORT_COMPILE_COMMANDS=ON. This creates acompile-commands.jsonfile which is used byclang-tidy.Steps for Testing
Edit in Editorbutton on the topclang-analyzer-cplusplus.Movecode issue is in the feedbackTestserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Test Coverage
Summary by CodeRabbit
New Features
Documentation
Tests