Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C#: Accept file sync mismatch for C# testfiles. #19199

Merged
merged 4 commits into from
Apr 4, 2025

Conversation

michaelnebel
Copy link
Contributor

To better allow inline expectations testing, we consider test files to be identical (or equivalent), if they are "the same" modulo comments.

@github-actions github-actions bot added the C# label Apr 2, 2025
@michaelnebel michaelnebel force-pushed the csharp/synctestfiles branch from 506ed74 to accee8b Compare April 2, 2025 13:22
@michaelnebel michaelnebel force-pushed the csharp/synctestfiles branch from 6e49dbf to 2193bec Compare April 2, 2025 13:31
@michaelnebel michaelnebel marked this pull request as ready for review April 3, 2025 06:05
@Copilot Copilot bot review requested due to automatic review settings April 3, 2025 06:06
@michaelnebel michaelnebel requested a review from a team as a code owner April 3, 2025 06:06
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates C# test file handling by allowing test file comparisons to ignore comment differences. Key changes include:

  • Removing a duplicate (and buggy) method in ConstantIfCondition.cs while providing a similar test case in ConstantConditionBad.cs.
  • Adding a test case for API abuse with missing Dispose calls in NoDisposeCallOnLocalIDisposableBad.cs.
  • Enhancing the sync-files.py script to support file equivalence modulo comment differences by introducing new functions and modifying check_group logic.

Reviewed Changes

Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantIfCondition.cs Removed a redundant Max method containing a bug.
csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantConditionBad.cs Added a test file that intentionally uses a buggy Max method.
csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposableBad.cs Introduced a test case highlighting missing Dispose call via an alert.
csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable.cs Removed duplicate faulty implementation of Bad class.
config/sync-files.py Modified file synchronization logic to consider files equivalent when differences are limited to comments.
Files not reviewed (2)
  • csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.expected: Language not supported
  • csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected: Language not supported
Comments suppressed due to low confidence (1)

config/sync-files.py:73

  • The parameter name 'accept_prefix' in check_group shadows the global function 'accept_prefix', which may cause confusion. Consider renaming the parameter to something like 'use_comment_check' or 'ignore_comments' for improved clarity.
def check_group(group_name, files, master_file_picker, emit_error, accept_prefix):

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

@michaelnebel michaelnebel merged commit 5a4e0ce into github:main Apr 4, 2025
16 checks passed
@michaelnebel michaelnebel deleted the csharp/synctestfiles branch April 4, 2025 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants