Programming exercises: Fix inconsistencies between diff viewer and diff line stats
#9984
Conversation
github-actions
bot
added
tests
server
magaupp
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
|
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/hestia/ProgrammingExerciseGitDiffReportIntegrationTest.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 changes involve modifications to several classes and methods related to git diff reporting in a programming exercise context. Key updates include alterations to method signatures in the Changes
Assessment against linked issues
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
|
![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
🧹 Outside diff range and nitpick comments (8)
src/main/java/de/tum/cit/aet/artemis/programming/web/GitDiffReportParserService.java (3)
33-35: Update Javadoc to include the newignoreWhitespaceparameterThe
extractDiffEntriesmethod signature has been updated to include theignoreWhitespaceparameter. The Javadoc should be updated to reflect this addition and explain its purpose for clarity and maintenance.Apply this diff to update the documentation:
/** * Extracts the ProgrammingExerciseGitDiffEntry from the raw git-diff output * * @param diff The raw git-diff output * @param useAbsoluteLineCount Whether to use absolute line count or previous line count + * @param ignoreWhitespace Whether to ignore entries where only leading and trailing whitespace differ * @return The extracted ProgrammingExerciseGitDiffEntries */
64-66: Consider refactoring duplication in diff handlersThe methods
handleAddition,handleRemoval, andhandleUnchangednow include additional logic that is somewhat repetitive. Refactor common functionality into a shared helper method to reduce code duplication and increase maintainability.
253-265: Initialize and clearaddedLinesandremovedLinesappropriatelyIn the
ParserStateclass, theaddedLinesandremovedLineslists are introduced for tracking changes. Make sure these lists are properly initialized and cleared at the right points in the parsing process to avoid residual data affecting subsequent diff entries.src/test/java/de/tum/cit/aet/artemis/programming/hestia/ProgrammingExerciseGitDiffReportServiceTest.java (1)
139-146: Enhance testgitDiffWhitespacewith additional assertionsThe new test
gitDiffWhitespaceverifies that no diff entries are created when only whitespace changes occur. To strengthen the test, add an assertion to confirm that the report itself is not null for completeness.Apply this diff to enhance the test:
@Test @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") void gitDiffWhitespace() throws Exception { exercise = hestiaUtilTestService.setupTemplate(FILE_NAME, " ", exercise, templateRepo); exercise = hestiaUtilTestService.setupSolution(FILE_NAME, "\t", exercise, solutionRepo); var report = reportService.updateReport(exercise); + assertThat(report).isNotNull(); assertThat(report.getEntries()).hasSize(0); }src/test/java/de/tum/cit/aet/artemis/programming/hestia/ProgrammingExerciseGitDiffReportIntegrationTest.java (3)
73-75: Refine assertions for report entries ingetGitDiffAsATutorThe test checks that
report.getEntries()is null. To improve clarity, consider checking if the entries are empty instead, as it provides better semantic meaning.Apply this diff:
assertThat(report).isNotNull(); -assertThat(report.getEntries()).isNull(); +assertThat(report.getEntries()).isNullOrEmpty();
84-86: Refine assertions for report entries ingetGitDiffAsAnEditorSimilar to the previous comment, adjust the assertion to check for empty entries.
Apply this diff:
assertThat(report).isNotNull(); -assertThat(report.getEntries()).isNull(); +assertThat(report.getEntries()).isNullOrEmpty();
95-97: Refine assertions for report entries ingetGitDiffAsAnInstructorAgain, adjust the assertion for consistency and clarity.
Apply this diff:
assertThat(report).isNotNull(); -assertThat(report.getEntries()).isNull(); +assertThat(report.getEntries()).isNullOrEmpty();src/main/java/de/tum/cit/aet/artemis/programming/service/hestia/ProgrammingExerciseGitDiffReportService.java (1)
237-237: Track LocalVC diff calculation enhancementThe TODO comment indicates a needed enhancement for calculating diffs in bare origin repository for LocalVC cases.
Would you like me to create a GitHub issue to track this enhancement? I can help draft the issue with:
- Description of the problem
- Technical requirements
- Implementation suggestions
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
src/main/java/de/tum/cit/aet/artemis/programming/service/CommitHistoryService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/hestia/ProgrammingExerciseGitDiffReportService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/GitDiffReportParserService.java(6 hunks)src/test/java/de/tum/cit/aet/artemis/atlas/UnionFindTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/hestia/ProgrammingExerciseGitDiffReportIntegrationTest.java(6 hunks)src/test/java/de/tum/cit/aet/artemis/programming/hestia/ProgrammingExerciseGitDiffReportServiceTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/java/de/tum/cit/aet/artemis/atlas/UnionFindTest.java
🧰 Additional context used
📓 Path-based instructions (5)
src/test/java/de/tum/cit/aet/artemis/programming/hestia/ProgrammingExerciseGitDiffReportServiceTest.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/hestia/ProgrammingExerciseGitDiffReportIntegrationTest.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/service/CommitHistoryService.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/hestia/ProgrammingExerciseGitDiffReportService.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/web/GitDiffReportParserService.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
🔇 Additional comments (11)
src/main/java/de/tum/cit/aet/artemis/programming/web/GitDiffReportParserService.java (4)
26-26: Verify the updated regex pattern for gitDiffLinePattern
The addition of .* at the end of the regex pattern gitDiffLinePattern allows matching additional characters after the @@ markers. Please ensure that this change correctly handles cases where extra information is appended to the diff hunk headers and doesn't inadvertently match unintended lines.
60-60: Pass ignoreWhitespace parameter to handleNewDiffBlock
In the call to handleNewDiffBlock, ensure that the ignoreWhitespace parameter is correctly passed to consistently handle whitespace-only changes within new diff blocks.
107-117: Ensure correct state management in handleUnchanged method
The handleUnchanged method resets certain parser state variables and calls finalizeEntry. Verify that this approach correctly handles transitions between diff entries, especially when ignoreWhitespace is enabled, to prevent skipping or misprocessing entries.
165-186:
Validate detection of whitespace-only changes
The new methods finalizeEntry and isWhitespaceOnlyChange are critical for filtering out entries that differ only by whitespace. Ensure that these methods accurately handle various cases, including different types of whitespace characters (spaces, tabs), line ending differences, and code with mixed indentation styles.
Consider adding comprehensive unit tests covering these scenarios to ensure reliability.
src/main/java/de/tum/cit/aet/artemis/programming/service/CommitHistoryService.java (1)
110-110: Clarify ignoreWhitespace setting in diff extraction
The extractDiffEntries method is called with ignoreWhitespace set to false. Confirm whether it's appropriate to consider whitespace changes in commit history diffs. If the intent is to ignore whitespace-only changes in this context, consider setting ignoreWhitespace to true.
src/test/java/de/tum/cit/aet/artemis/programming/hestia/ProgrammingExerciseGitDiffReportIntegrationTest.java (1)
107-110: Ensure appropriate access to report entries in getGitDiffBetweenTemplateAndSubmission
As an instructor, you may expect the report entries to be available. If getEntries() is null, verify whether this is the intended behavior or if entries should be accessible in this context.
src/main/java/de/tum/cit/aet/artemis/programming/service/hestia/ProgrammingExerciseGitDiffReportService.java (5)
112-112: LGTM! Style improvement
Removing redundant this keyword improves code readability.
167-167: LGTM! Consistent style improvement
310-312: LGTM! Improved parameter naming
The parameter names have been updated to be more descriptive and self-documenting:
repo1→firstRepooldTreeParser→firstRepoTreeParsernewTreeParser→secondRepoTreeParser
318-323: Verify opposite whitespace handling parameter
The whitespace handling parameter is set to true here (extractDiffEntries(diff, false, true)), opposite to line 218 (extractDiffEntries(diff, true, false)). This difference needs verification.
Let's analyze the impact:
✅ Verification successful
Different whitespace handling is intentional and correct
The method signature in GitDiffReportParserService shows that ignoreWhitespace parameter determines "Whether to ignore entries where only leading and trailing whitespace differ". The different usage is intentional:
- In
createReport: UsesignoreWhitespace=truebecause it's generating a diff report for display/review purposes where whitespace-only changes are less relevant - In line 218: Uses
ignoreWhitespace=falsebecause it's calculating the total number of diff lines for comparison purposes where every change, including whitespace, should be counted
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the context of different whitespace parameter usage
# Expected: Different whitespace handling for different use cases
# Search for the method definition to understand parameter meaning
ast-grep --pattern 'extractDiffEntries($diff, $count, $whitespace) {
$$$
}'
# Search for all usages with context
rg -U "extractDiffEntries\([^)]+\)" --type java -B 5 -A 5
Length of output: 7298
218-218: Verify whitespace handling parameters
The new parameter added to extractDiffEntries(diff, true, false) affects whitespace handling in diffs. Please verify:
- First boolean (
true): Appears to control line counting - Second boolean (
false): Appears to control whitespace ignoring
Let's verify the parameter usage across the codebase:
✅ Verification successful
Let me check the implementation of extractDiffEntries to understand the parameter usage:
Let me check the usage of these parameters in the helper methods to fully understand their impact:
Based on the implementation and usage across the codebase, I can now provide a final response:
Parameters in extractDiffEntries are used correctly
The parameters in extractDiffEntries(diff, true, false) are correctly used in this context:
useAbsoluteLineCount=true: Used to count lines in the current version of the file, which is appropriate for calculating total line changesignoreWhitespace=false: Ensures whitespace changes are counted, which is correct for accurate line counting
Other usages in the codebase are also consistent with their purposes:
extractDiffEntries(diff, false, true): Used when whitespace should be ignored and line counting is relativeextractDiffEntries(diffs.toString(), false, false): Used when both absolute counting and whitespace changes should be considered
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check all calls to extractDiffEntries to ensure consistent parameter usage
# Expected: All calls should have consistent whitespace handling parameters
# Search for all method calls
rg -U "extractDiffEntries\([^)]+\)" --type java
Length of output: 975
Script:
#!/bin/bash
# Get the implementation of extractDiffEntries and surrounding context
ast-grep --pattern 'public List<ProgrammingExerciseGitDiffEntry> extractDiffEntries(String diff, boolean useAbsoluteLineCount, boolean ignoreWhitespace) {
$$$
}'
Length of output: 6797
Script:
#!/bin/bash
# Get the implementation of handleUnchanged and handleRemoval methods
ast-grep --pattern 'private void handleUnchanged($$$) {
$$$
}'
ast-grep --pattern 'private void handleRemoval($$$) {
$$$
}'
Length of output: 6194
.../tum/cit/aet/artemis/programming/hestia/ProgrammingExerciseGitDiffReportIntegrationTest.java
Outdated
Show resolved
Hide resolved
.../tum/cit/aet/artemis/programming/hestia/ProgrammingExerciseGitDiffReportIntegrationTest.java
Outdated
Show resolved
Hide resolved
HawKhiem
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
eceeeren
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Works in TS6, works as expected!
Just as a side note: I would prefer if you could create testing steps a bit more detailed and maybe add screenshots :) I think not everyone knows where every button is placed in the application. For example, for a long time, I searched for the "Review Changes" button. It also could be nice to see the expected output. Just a friendly request :)
sawys777
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
alekspetrov9e
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Server
Changes affecting Programming Exercises
Motivation and Context
The Monaco Editor ignores leading and trailing whitespace while computing the diff (see ignoreTrimWhitespace).
The displayed counts of added and removed lines are provided by the server.
The server-side diff does not ignore whitespace like the Monaco Editor.
The git-diff output allows its hunk headers to be annotated with the containing function if the
diffattribute is set accordingly (see custom hunk headers).Such blocks are not handled correctly by the server parser and are ignored.
Description
The GitDiffReportParserService is modified to optionally ignore diffs where the lines only differ in leading and trailing whitespace.
This option to ignore whitespace is enabled where the diff is used together with the diff viewer.
Hunk headers are handled correctly.
Closes #9928.
Steps for Testing
Testserver 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
Code Review
Manual Tests
Test Coverage
Server
Screenshots
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores