Programming exercises: Add Ruff SCA for Python to LocalCI
#9573
Conversation
github-actions
bot
added
tests
server
magaupp
changed the title
Programming exercises: Add Ruff SCA for PythonProgramming exercises: Add Ruff SCA for Python to LocalCI
magaupp
force-pushed
the
feature/programming-exercises/python-sca
branch
from
41becc3 to
9802cae
Compare
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmd (7.7.0)src/test/java/de/tum/cit/aet/artemis/programming/StaticCodeAnalysisParserUnitTest.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. WalkthroughThe pull request introduces enhancements to support Python-specific static code analysis within the existing framework. Key additions include the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
magaupp
force-pushed
the
feature/programming-exercises/python-sca
branch
from
9802cae to
9d809ef
Compare
magaupp
changed the base branch from
develop
to
feature/programming-exercises/sarif-parser
magaupp
force-pushed
the
feature/programming-exercises/python-sca
branch
from
70fbb8f to
7cc3748
Compare
magaupp
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
src/main/resources/templates/aeolus/python/default_static.sh (3)
1-3: Consider hardening the shell script setupWhile the basic setup is good, consider adding these safety flags for more robust error handling:
#!/usr/bin/env bash -set -e +set -euo pipefail export AEOLUS_INITIAL_DIRECTORY=${PWD}
18-30: Improve directory handling safetyThe current directory handling could be safer with proper error checking and cleanup.
main () { if [[ "${1}" == "aeolus_sourcing" ]]; then return 0 # just source to use the methods in the subshell, no execution fi local _script_name _script_name=${BASH_SOURCE[0]:-$0} + + # Ensure we return to initial directory on exit + trap 'cd "${AEOLUS_INITIAL_DIRECTORY}"' EXIT + + # Validate directory exists + if [[ ! -d "${AEOLUS_INITIAL_DIRECTORY}" ]]; then + echo "Error: Initial directory not found" >&2 + return 1 + fi + cd "${AEOLUS_INITIAL_DIRECTORY}" bash -c "source ${_script_name} aeolus_sourcing; static_code_analysis" - cd "${AEOLUS_INITIAL_DIRECTORY}" bash -c "source ${_script_name} aeolus_sourcing; build_and_test_the_code" }
1-30: Consider separating configuration from executionWhile the script implements the required functionality for Python static code analysis, consider these architectural improvements:
- Move the Ruff configuration options to a separate configuration file
- Add support for environment variables to override default settings
- Consider implementing a logging mechanism for better debugging
This would improve maintainability and flexibility of the static analysis implementation.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 6-6: studentParentWorkingDirectoryName is referenced but not assigned.
(SC2154)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseRepositoryService.java (2)
119-120: Consider refactoringRepositoryResourcesfor better maintainabilityThe
RepositoryResourcesrecord now contains seven fields, which might make it harder to manage and understand. Consider encapsulating related fields into separate records or classes to improve code clarity and maintainability.
334-340: Enhance comment clarity for resource copying sectionsTo improve readability, consider separating comments for the project type and static code analysis resource copying sections:
// Copy project type specific files if (repositoryResources.projectTypeResources != null) { fileService.copyResources(repositoryResources.projectTypeResources, repositoryResources.projectTypePrefix, repoLocalPath, true); } // Copy static code analysis specific files if (repositoryResources.staticCodeAnalysisResources != null) { fileService.copyResources(repositoryResources.staticCodeAnalysisResources, repositoryResources.staticCodeAnalysisPrefix, repoLocalPath, true); }This makes it clear that the two blocks handle different resource types.
src/test/resources/test-data/static-code-analysis/expected/ruff.json (1)
5-13: Use relative file paths in test data for portabilityUsing absolute file paths like
/usr/local/src/exercise/policy.pyin test data may lead to issues when running tests in different environments. Consider replacing them with relative paths to improve portability:"filePath": "exercise/policy.py",This change ensures that the test data remains valid regardless of the file system structure.
src/main/java/de/tum/cit/aet/artemis/core/config/StaticCodeAnalysisConfigurer.java (1)
17-78: Consider prioritizing categories with different penalty weightsThe comprehensive list of Python categories is good, but consider grouping them by severity and assigning different penalty weights. For example:
- Critical (higher penalty): security-related categories like "flake8-bandit"
- Major (medium penalty): code correctness like "Pyflakes"
- Minor (lower penalty): style-related like "pycodestyle"
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java (1)
344-344: Use a more appropriate default category for RUFFUsing "Pylint" as the category for RUFF tool might be confusing since they are different tools. Consider using a more generic category like "Code Quality" or one of RUFF's native categories.
- case RUFF -> "Pylint"; + case RUFF -> "Pyflakes"; // Using Pyflakes as it's one of RUFF's core rules
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
src/main/resources/config/application.ymlis excluded by!**/*.ymlsrc/main/resources/templates/aeolus/python/default_static.yamlis excluded by!**/*.yamlsrc/main/resources/templates/python/staticCodeAnalysis/test/ruff-student.tomlis excluded by!**/*.toml
📒 Files selected for processing (11)
src/main/java/de/tum/cit/aet/artemis/core/config/StaticCodeAnalysisConfigurer.java(2 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/ProgrammingExerciseRepositoryService.java(6 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(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/sarif/RuffCategorizer.java(1 hunks)src/main/resources/templates/aeolus/python/default_static.sh(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/ruff.json(1 hunks)src/test/resources/test-data/static-code-analysis/reports/ruff.sarif(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java (1)
Pattern 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.java (1)
Pattern 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/main/java/de/tum/cit/aet/artemis/programming/domain/StaticCodeAnalysisTool.java (1)
Pattern 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/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java (1)
Pattern 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/programming/service/localci/scaparser/strategy/ParserPolicy.java (1)
Pattern 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/programming/service/localci/scaparser/strategy/sarif/RuffCategorizer.java (1)
Pattern 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/programming/service/ProgrammingExerciseRepositoryService.java (1)
Pattern 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.java (1)
Pattern 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
📓 Learnings (1)
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/python/default_static.sh
[warning] 6-6: studentParentWorkingDirectoryName is referenced but not assigned.
(SC2154)
🔇 Additional comments (15)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseRepositoryService.java (6)
52-53: Definition of STATIC_CODE_ANALYSIS_DIR constant is appropriate
The addition of the STATIC_CODE_ANALYSIS_DIR constant enhances code maintainability by centralizing the directory name.
134-134: Variable renaming enhances clarity
Renaming projectTypeTemplateDir to repositoryTypeTemplateDir improves readability by accurately reflecting the variable's purpose.
141-141: Path resolution uses the correct template directory
The use of repositoryTypeTemplateDir in path resolution ensures that the correct repository type templates are accessed.
144-144: Prefix path correctly incorporates repository type
The prefix variable now accurately includes the repository type, aligning with the updated directory structure.
155-156: Project type paths are correctly resolved
The variables projectTypeSpecificPrefix and projectTypeTemplatePath correctly use repositoryTypeTemplateDir, ensuring proper path resolution for project-specific resources.
171-183: Static code analysis resources are conditionally loaded
The addition of static code analysis resources is correctly implemented with a conditional check, ensuring resources are only loaded when static code analysis is enabled.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/ParserPolicy.java (1)
32-32: Integration of RUFF parser is correctly implemented
The addition of the RUFF case with a new SarifParser and RuffCategorizer correctly integrates RUFF support into the parser configuration.
src/test/resources/test-data/static-code-analysis/expected/ruff.json (1)
54-56: Handle unknown categories appropriately in test data
The rule X999 with category "Unknown" is correctly included to test how the system handles unknown categories, ensuring robustness in the parsing logic.
src/main/java/de/tum/cit/aet/artemis/programming/domain/StaticCodeAnalysisTool.java (1)
21-21: RUFF tool added to the StaticCodeAnalysisTool enum and mapped correctly
The addition of RUFF to the enum and its association with ProgrammingLanguage.PYTHON appropriately extends static code analysis support for Python exercises.
Also applies to: 31-31
src/test/java/de/tum/cit/aet/artemis/programming/StaticCodeAnalysisParserUnitTest.java (1)
77-80: LGTM! Test follows established patterns
The new test method is well-structured, follows the existing test patterns, and maintains consistency with other parser tests in the class.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java (1)
56-56: LGTM! Python features correctly updated for SCA support
The update to Python's language features correctly enables static code analysis support, aligning with the PR's objective of adding Ruff SCA.
src/test/resources/test-data/static-code-analysis/reports/ruff.sarif (3)
1-3: LGTM! Valid SARIF schema reference
The file correctly references the SARIF 2.1.0 schema, ensuring proper format validation.
5-115: Comprehensive test cases for Python static analysis
The test cases cover important Python-specific issues:
- Wildcard imports (F403)
- Magic numbers (PLR2004)
- Undefined names from star imports (F405)
- Generic error case (X999)
This provides good coverage for testing the Ruff parser implementation.
117-190: Well-documented rules with detailed explanations
Each rule includes:
- Full descriptions with examples
- Help text
- Documentation links
- Severity levels
- Rule categories
This comprehensive documentation will help developers understand and fix the reported issues.
src/main/java/de/tum/cit/aet/artemis/core/config/StaticCodeAnalysisConfigurer.java (1)
82-83: LGTM!
The addition of Python to the language-specific categories map is clean and follows the existing pattern.
...um/cit/aet/artemis/programming/service/localci/scaparser/strategy/sarif/RuffCategorizer.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/config/StaticCodeAnalysisConfigurer.java
Show resolved
Hide resolved
# Conflicts: # src/test/java/de/tum/cit/aet/artemis/programming/StaticCodeAnalysisParserUnitTest.java
SimonEntholzer
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
SimonEntholzer
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Server
Changes affecting Programming Exercises
Motivation and Context
Static Code Analysis is currently not supported for Python.
Description
This PR adds the Ruff static code analysis tool for Python.
Each issue is categorized according to its Ruff tool.
The PR also enables file overrides for static code analysis in exercise templates.
Steps for Testing
Prerequisites:
Pythonas the languageEnable Static Code AnalysisGradingbuttonCode AnalysistabTestserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Server
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation