885 static analyser framework checkstyle support #897
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… and robust metric application tests Implemented a new Checkstyle XML report integration including a dedicated CheckstyleParsingConfig (XPath mapping for <file>/<error> nodes) and parser setup to produce file-level aggregates plus minimal per-error context findings. Refactored Java node indexing by splitting the former Java index strategy into tool-specific implementations, so each tool (e.g., Checkstyle, JaCoCo) controls its own path → node-id normalization rules. Added source-root based path normalization for Checkstyle: CheckstyleIndexNodeStrategy now relies on ParsingConfig.SourceRootRelativePath(...) (driven by SourceRootMarker) to make GLX and report paths match even when absolute prefixes differ. JaCoCo remains unchanged since it already normalizes paths; added a note to re-check if extra handling is needed when GLX paths diverge. Improved metric application behavior: when a node already contains the same metric key, additional values are stored under "<key> [i]" instead of overwriting, enabling multiple findings to coexist safely. Updated the shared test harness (TestReportGraphProviderBase) for the new behavior: node lookup is no longer context-dependent; it uses SourceRangeIndex when a StartLine is available and falls back to TryGetNode otherwise. Metric assertions now search both base keys and all suffixed variants. Added a concrete Checkstyle test fixture with representative expected findings and placed example Checkstyle report + matching GLX under StreamingAssets/checkstyle to make the end-to-end tests reproducible.
…sible - Drop XPathMapping.MapContext and treat the XML tag name as parsing context. - Refactor JaCoCo/Checkstyle configs to rely on MetricsByContext per context (remove NaN/guard XPath hacks), introduce context constants and align PathBuilders/FileName/Location mappings. - Format numeric XPath results using invariant culture to avoid locale-dependent metric strings. - Harden XML reader settings (ignore DTDs, disable XmlResolver). - Make MetricApplier stateless by removing the static prefix cache and passing the prefix explicitly. - Add SourceRootRelativePath() and persist SourceRootMarker in ParsingConfig serialization; restore configs via ParsingConfigFactory. - Add SaveAdditional/RestoreAdditional hooks to ParsingConfig to support derived config serialization. - Clean up Checkstyle index strategy debug output and update tests/fixtures accordingly. BREAKING: - Finding.Metrics is now Dictionary<string,string> (no nullable values). - Checkstyle issue metric key renamed from 'Context-Level.Issue' to 'ContextLevel.Issue'. - ParsingConfig.Restore is now static and factory-based.
…ogic changes) Add XML documentation for all declarations (including private/protected) and document namespaces. Apply naming rules consistently: private fields/constants start lower-case, remove underscore separators, normalize local names (e.g., ComputeKey, xpathMapping). mark unused parameters/out values as discards (_, out _). Enforce style rules: add braces to all if/loop bodies. replace var with explicit types (keep explicit KeyValuePair<,> for dictionary iteration). Improve code hygiene without changing behavior: remove unused usings and unnecessary temporaries. fix ArgumentNullException usage to use nameof(...). add null-guards consistent with documented preconditions. Fix invalid XML doc markup by escaping <file>/<error> tags. Normalize formatting/whitespace (regions, XPath string spacing) and mark non-null intent where required.
felarmouche
added
the
bachelorproject
koschke
removed
the
bachelorproject
…-framework-checkstyle-support # Conflicts: # Assets/StreamingAssets/UserSettings.cfg
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds Checkstyle XML report support to the SEE static analysis framework and refactors the XML parsing infrastructure to be context-aware, making it easier to extend for new tools while improving robustness.
Key Changes
- Introduces Checkstyle parsing configuration with file-level aggregated metrics and error-level issue tracking
- Refactors XPath mapping to use context-aware metrics (
MetricsByContext) instead of global metrics, avoiding NaN evaluation for irrelevant contexts - Adds source root path normalization (
SourceRootMarker) to handle absolute paths from different tools and environments
Reviewed changes
Copilot reviewed 23 out of 26 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
CheckstyleParsingConfig.cs |
New configuration for Checkstyle reports with aggregated file metrics and per-error issue strings |
CheckstlyeIndexNodeStrategy.cs |
New strategy for mapping Checkstyle file paths to Java FQCNs (filename has typo) |
XmlReportParser.cs |
Enhanced with context-aware metric resolution, safer XML defaults, and invariant culture parsing |
XPathMapping.cs |
Refactored to use MetricsByContext dictionary instead of flat Metrics dictionary |
ParsingConfig.cs |
Added SourceRootMarker field and SourceRootRelativePath() helper; made Restore() static |
MetricApplier.cs |
Made static, added key disambiguation for duplicate metrics |
JaCoCoParsingConfig.cs |
Updated to use MetricsByContext structure |
JaCoCoIndexNodeStrategy.cs |
Renamed from JavaIndexNodeStrategy, added Windows path separator handling |
TestCheckstyleReport.cs |
New integration test for Checkstyle parsing |
TestReportGraphProviderBase.cs |
Enhanced documentation and metric validation logic |
| Unity meta files | Standard Unity metadata for new assets |
checkstyle-result.xml |
Sample Checkstyle report for testing |
SEE.cfg |
Updated to reference Checkstyle test data |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Assets/SEETests/TestReportGraphProvider/TestReportGraphProviderBase.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
koschke
requested changes
Dec 17, 2025
Collaborator
koschke
left a comment
koschke
left a comment
There was a problem hiding this comment.
Thanks for the PR. Looks very good overall. I have noted only a few minor issues to be fixed.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
No real change beyond formatting.
koschke
approved these changes
Dec 18, 2025
Collaborator
koschke
left a comment
koschke
left a comment
There was a problem hiding this comment.
Looks good. Can be merged.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces Checkstyle report support and refactors the XML report parsing/config layer to be context-aware, easier to extend, and more robust—without changing existing parsing logic.
Key changes
Checkstyle support
CheckstyleParsingConfig(ToolId = "Checkstyle") andCheckstlyeIndexNodeStrategy<file>paths to Java FQCN nodes (handles Windows/Linux separators, source-root stripping viaSourceRootMarker)XML parsing refactor (extensibility/robustness)
XPathMappingtoMetricsByContext(context = XML tag name) to avoid applying irrelevant metrics.XmlReportParserto:current.LocalNamecontextDtdProcessing = Ignore,XmlResolver = null)InvariantCultureConfig + indexing improvements
ParsingConfigwithSourceRootMarker+ helperSourceRootRelativePath(fullPath)(path normalization + optional root stripping)ParsingConfig.Restore(...)static (instantiates via factory; supports derived configs via hooks)JavaIndexNodeStrategy→JaCoCoIndexNodeStrategy(behavior unchanged; adds Windows separator handling)Findings / metrics application
Finding.Metricsis always non-null (default dictionary)MetricApplierstatic, improve validation, and avoid overwriting duplicate metric keys by storing them as:Key,Key [1],Key [2], ...Tests + samples
TestCheckstyleReportintegration-style testcheckstyle-result.xml,checkstyle.glx.xz) and Unity meta files