Programming exercises: Enable static code analysis for Dart with LocalCI
#10354
Conversation
github-actions
bot
added
tests
server
magaupp
force-pushed
the
feature/programming-exercises/dart-sca
branch
from
b18233c to
e8e8f9f
Compare
github-actions
bot
removed
the
client
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
WalkthroughThis pull request extends support for Dart within continuous integration and static analysis systems. It updates configuration files, adds new constants and enum entries for Dart analysis support, and modifies control flow in the parser to handle Dart reports using a newly introduced categorizer. A Bash script is added to automate Dart project management tasks, and supplementary tests and test data are provided to validate the Dart static analysis workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI System
participant LocalCI as LocalCIService
participant Parser as ParserPolicy
participant Categorizer as DartAnalyzeCategorizer
participant Tester as TestSuite
CI->>LocalCI: Trigger Dart build with static analysis
LocalCI->>Parser: Request parser for 'dart_analyze.sarif'
Parser->>Categorizer: Instantiate categorizer for DART_ANALYZE case
Categorizer-->>Parser: Return categorized rule information
Parser-->>LocalCI: Return parsed analysis results
Tester->>LocalCI: Execute tests (e.g., testDartAnalyzeParser)
Suggested labels
Suggested reviewers
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. 🔧 Shellcheck (0.10.0)src/main/resources/templates/aeolus/dart/default_static.shsrc/main/resources/templates/aeolus/dart/default_static.sh: src/main/resources/templates/aeolus/dart/default_static.sh: openBinaryFile: does not exist (No such file or directory) ✨ Finishing Touches
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/sarif/DartAnalyzeCategorizer.java (1)
16-19: Consider adding validation for known rule types.While the implementation is correct, consider validating against a set of known types to prevent potential issues with unexpected values.
@Override public String categorizeRule(ReportingDescriptor rule) { + private static final Set<String> KNOWN_TYPES = Set.of( + "TODO", "HINT", "COMPILE_TIME_ERROR", + "CHECKED_MODE_COMPILE_TIME_ERROR", "STATIC_WARNING", + "SYNTACTIC_ERROR", "LINT" + ); Map<String, Object> properties = rule.getOptionalProperties().map(PropertyBag::additionalProperties).orElseGet(Map::of); - return properties.getOrDefault("type", "UNKNOWN").toString(); + String type = properties.getOrDefault("type", "UNKNOWN").toString(); + return KNOWN_TYPES.contains(type) ? type : "UNKNOWN"; }src/test/resources/test-data/static-code-analysis/expected/dart_analyze.json (1)
1-17: JSON Structure and Schema ValidityThe JSON file is well-structured and clearly represents a Dart static analysis result. The keys such as
"tool","issues","filePath","startLine","endLine","startColumn","endColumn","rule","category","message", and"priority"are all consistently named and follow a clear schema.Consider adding additional metadata (e.g., a tool version or timestamp) if future extensibility or traceability is desired.
src/main/resources/templates/aeolus/dart/default_static.sh (2)
1-3: Script Initialization and RobustnessThe use of the shebang (
#!/usr/bin/env bash) along withset -eensures that the script fails on errors, which is good practice.Consider adding
set -o pipefailimmediately afterset -eto ensure that failures in any part of a pipeline are correctly propagated.Proposed diff:
-#!/usr/bin/env bash -set -e +#!/usr/bin/env bash +set -e +set -o pipefailThis change would help catch failures in multi-command pipelines.
27-41: Function: main and Subshell InvocationThe
mainfunction orchestrates the execution by sourcing the script itself with theaeolus_sourcingparameter before calling each function. This pattern ensures that functions are available in subshells and that the working directory is reset between tasks.It is advisable to wrap the script name (
${_script_name}) in quotes when used in the subshell commands to safeguard against potential issues with spaces in the path.
Proposed diff for one of the subshell calls (apply similarly to lines 34, 36, 38, and 40):- bash -c "source ${_script_name} aeolus_sourcing; install_student_dependencies" + bash -c "source \"${_script_name}\" aeolus_sourcing; install_student_dependencies"This change will improve the robustness of the script in environments where the script path might contain spaces.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
src/main/resources/config/application.ymlis excluded by!**/*.ymlsrc/main/resources/templates/aeolus/dart/default_static.yamlis excluded by!**/*.yamlsrc/main/resources/templates/dart/staticCodeAnalysis/exercise/analysis_options.yamlis excluded by!**/*.yamlsrc/main/resources/templates/dart/staticCodeAnalysis/solution/analysis_options.yamlis excluded by!**/*.yamlsrc/main/resources/templates/dart/staticCodeAnalysis/test/analysis_options.yamlis excluded by!**/*.yaml
📒 Files selected for processing (11)
docs/user/exercises/programming-exercise-features.inc(1 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(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/sarif/DartAnalyzeCategorizer.java(1 hunks)src/main/resources/templates/aeolus/dart/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/dart_analyze.json(1 hunks)src/test/resources/test-data/static-code-analysis/reports/dart_analyze.sarif(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/resources/test-data/static-code-analysis/reports/dart_analyze.sarif
🧰 Additional context used
📓 Path-based instructions (2)
`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/programming/service/localci/LocalCIProgrammingLanguageFeatureService.javasrc/main/java/de/tum/cit/aet/artemis/programming/domain/StaticCodeAnalysisTool.javasrc/main/java/de/tum/cit/aet/artemis/core/config/StaticCodeAnalysisConfigurer.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/DartAnalyzeCategorizer.java
`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
🧠 Learnings (3)
src/main/java/de/tum/cit/aet/artemis/core/config/StaticCodeAnalysisConfigurer.java (2)
Learnt from: magaupp
PR: ls1intum/Artemis#10204
File: src/main/java/de/tum/cit/aet/artemis/core/config/StaticCodeAnalysisConfigurer.java:164-165
Timestamp: 2025-01-28T17:45:27.844Z
Learning: In StaticCodeAnalysisConfigurer, default categories for single-tool configurations (like Ruff and RuboCop) should use CategoryState.FEEDBACK with minimal penalties (penalty: 0.0, maxPenalty: 1.0) to ensure feedback is immediately visible to users.
Learnt from: magaupp
PR: ls1intum/Artemis#9573
File: src/main/java/de/tum/cit/aet/artemis/core/config/StaticCodeAnalysisConfigurer.java:154-157
Timestamp: 2024-12-06T01:01:18.563Z
Learning: In `StaticCodeAnalysisConfigurer.java`, default penalties and category states for static code analysis categories should not be set in the code, as these should be configured by instructors using the UI on a case-by-case basis.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/ParserPolicy.java (2)
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.
Learnt from: magaupp
PR: ls1intum/Artemis#10204
File: src/main/java/de/tum/cit/aet/artemis/core/config/StaticCodeAnalysisConfigurer.java:164-165
Timestamp: 2025-01-28T17:45:27.844Z
Learning: In StaticCodeAnalysisConfigurer, default categories for single-tool configurations (like Ruff and RuboCop) should use CategoryState.FEEDBACK with minimal penalties (penalty: 0.0, maxPenalty: 1.0) to ensure feedback is immediately visible to users.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/sarif/DartAnalyzeCategorizer.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.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-tests
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (14)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/sarif/DartAnalyzeCategorizer.java (1)
8-13: LGTM! Clear and comprehensive documentation.The Javadoc clearly lists all possible rule types that can be returned by the categorizer.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/ParserPolicy.java (1)
32-32: LGTM! Consistent with existing patterns.The new case for DART_ANALYZE follows the established pattern for SARIF-based tools.
src/main/java/de/tum/cit/aet/artemis/programming/domain/StaticCodeAnalysisTool.java (2)
18-18: LGTM! Follows established naming patterns.The new enum constant follows the existing naming convention and uses the SARIF file extension consistently with other SARIF-based tools.
33-33: LGTM! Correctly placed mapping.The Dart language mapping is correctly placed in alphabetical order and follows the existing pattern.
src/test/java/de/tum/cit/aet/artemis/programming/StaticCodeAnalysisParserUnitTest.java (1)
61-64: LGTM! Test follows established patterns.The test method is well-placed alphabetically and follows the same pattern as other SARIF tool tests.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java (1)
67-67: LGTM! Dart feature configuration is consistent.The updated configuration enables static code analysis for Dart while maintaining other feature flags appropriately.
src/main/java/de/tum/cit/aet/artemis/core/config/StaticCodeAnalysisConfigurer.java (1)
85-86: LGTM! Implementation aligns with established patterns.The changes follow the established patterns for configuring static code analysis tools:
- Comprehensive category list covering all Dart analyzer output types
- Consistent with single-tool configuration pattern using CategoryState.FEEDBACK
- Avoids hardcoding specific penalties, allowing instructor configuration via UI
Also applies to: 91-91
docs/user/exercises/programming-exercise-features.inc (1)
108-108: LGTM! Documentation accurately reflects implementation.The feature matrix update correctly shows the differential support for Dart static code analysis:
- Local CI: Enabled (L: yes)
- Jenkins: Disabled (J: no)
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java (1)
356-356: LGTM! Default category selection is appropriate.Using "LINT" as the default category for Dart analysis issues is a sensible choice, as it's the most general category from the available set and matches the pattern used by other tools like RuboCop.
src/main/resources/templates/aeolus/dart/default_static.sh (5)
4-8: Function: install_student_dependenciesThe function correctly echoes a status message, changes directories based on the variable
studentParentWorkingDirectoryName, and runsdart pub get.Ensure that the variable
studentParentWorkingDirectoryNameis properly defined in the environment or passed in from the caller.
10-14: Function: install_dependenciesSimilarly, this function is well-structured for installing dependencies in the test working directory.
Ensure that the
testWorkingDirectoryvariable is set correctly in the context where this script is used.
16-19: Function: static_code_analysisThe function executes Dart’s static analysis tool and pipes the JSON output directly into the
analyze_sarifprocessor. The use of command-line arguments to set the source root and output file is clear and aligns with the PR objectives.Confirm that the
analyze_sariftool is available in the execution environment and that its parameters correctly match the expected SARIF conversion requirements.
21-25: Function: testThe function runs Dart tests with a JSON reporter and processes the output to generate an XML report using
tojunitandxmlstarlet.Ensure that both
tojunitandxmlstarletare installed in environments where this script will be executed, as missing dependencies might cause failures.
43-44: Script ExecutionThe script correctly invokes the
mainfunction with all command-line arguments, ensuring that its functionality is properly executed when run.
helios-aet
bot
temporarily deployed
to
artemis-test7.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test9.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
Important
testGetFinishedBuildJobs_returnsFilteredJobscurrently fails on developChecklist
General
Server
Changes affecting Programming Exercises
Description
This PR adds static code analysis for Dart.
No tool existed for converting
dart analyzes output to SARIF, so I created one for this feature:https://github.com/ls1intum/dart_analyze_sarif
The issue category is provided by
dart analyzeand can be one ofTODO,HINT,COMPILE_TIME_ERROR,CHECKED_MODE_COMPILE_TIME_ERROR,STATIC_WARNING,SYNTACTIC_ERRORorLINT.Steps for Testing
Prerequisites:
;) to a function of the solution repositoryempty_statementscode issue is in the Solution 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
Documentation
New Features
Tests