Cache removed and filelist unit test #32

SeanTConrad · 2024-12-13T15:35:39Z

Summary by Sourcery

Add a unit test for file list processing and remove caching from the main function to streamline the code.

Enhancements:

Remove the caching mechanism from the main function in modernmetric to simplify the code.
Updated workflows so coverage runs. Codecov upload is failing, but that's because of the token
New unit test found some bugs in Python 3.9 with "|" operator that I fixed

Tests:

Add a new unit test 'test_filelist_scan' to verify the functionality of processing a JSON file list of file paths.

sourcery-ai · 2024-12-13T15:35:43Z

Reviewer's Guide by Sourcery

This PR removes the caching functionality and adds a unit test for the file list feature. The main changes involve removing cache-related code from the main module and adding a new test case that verifies the file list scanning functionality.

Class diagram for ArgParser changes

classDiagram
    class ArgParser {
        +--file: str
        +files: str[]
    }
    note for ArgParser "Updated argument definitions for file and files"

Class diagram for main function changes

classDiagram
    class MainFunction {
        -cache: Cache
        +_result: dict
        +_importer: dict
        +results: list
    }
    note for MainFunction "Removed cache-related code and updated processing logic"

File-Level Changes

Change	Details	Files
Removed caching functionality from the main processing logic	Commented out cache initialization code Removed cache parameter from file_process function call	`modernmetric/__main__.py`
Added new unit test for file list scanning feature	Created test_filelist_scan function to verify file list processing Added assertions to verify file statistics and overall metrics Implemented cleanup of test output files	`test/test_self_scan.py`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time. You can also use
this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey @SeanTConrad - I've reviewed your changes - here's some feedback:

Overall Comments:

The cache functionality appears to be commented out without explanation. Please either remove the cache code entirely if it's no longer needed, or explain why it's being disabled in the commit message.

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟡 Testing: 2 issues found
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

sourcery-ai · 2024-12-13T15:36:55Z

test/test_self_scan.py

+def test_filelist_scan():
+    curr_dir = os.path.dirname(os.path.abspath(__file__))
+    project_root = os.path.dirname(curr_dir)
+    stats_input_file = os.path.join(
+        project_root, 'testfiles', 'samplefilelist.json'
+    )
+    stats_output_file = os.path.join(curr_dir, "test.stats.json")
+    custom_args = [
+        f"--file={stats_input_file}",
+        f"--output={stats_output_file}"


suggestion (testing): Test should verify file list processing functionality more thoroughly

While the test checks basic file metrics, it should also verify: 1) handling of invalid file paths in the JSON, 2) empty file lists, 3) proper error handling for missing or malformed JSON files, and 4) relative/absolute path handling.

Suggested implementation:

def create_temp_json_file(content, tmp_path): temp_file = tmp_path / "test_filelist.json" with open(temp_file, 'w') as f: json.dump(content, f) return str(temp_file) def run_modernmetric_with_files(input_file, tmp_path): output_file = str(tmp_path / "output.stats.json") custom_args = [ f"--file={input_file}", f"--output={output_file}" ] return modernmetric(custom_args=custom_args, license_identifier='unit_test'), output_file def test_filelist_scan_valid_files(tmp_path): """Test processing of valid file list""" curr_dir = os.path.dirname(os.path.abspath(__file__)) project_root = os.path.dirname(curr_dir) stats_input_file = os.path.join( project_root, 'testfiles', 'samplefilelist.json' ) _, output_file = run_modernmetric_with_files(stats_input_file, tmp_path) with open(output_file, 'r') as f: stats = json.load(f) files = stats['files'] assert files is not None assert files["../testfiles/test.c"]["loc"] == 25 assert files["../testfiles/test.c"]["cyclomatic_complexity"] == 0 assert stats["overall"]["loc"] == 178 def test_filelist_scan_invalid_paths(tmp_path): """Test handling of invalid file paths in JSON""" invalid_files = { "files": [ "nonexistent_file.c", "/invalid/path/file.cpp", "../../../outside/scope.py" ] } input_file = create_temp_json_file(invalid_files, tmp_path) _, output_file = run_modernmetric_with_files(input_file, tmp_path) with open(output_file, 'r') as f: stats = json.load(f) assert stats['files'] == {} assert stats["overall"]["loc"] == 0 def test_filelist_scan_empty_list(tmp_path): """Test handling of empty file list""" empty_files = {"files": []} input_file = create_temp_json_file(empty_files, tmp_path) _, output_file = run_modernmetric_with_files(input_file, tmp_path) with open(output_file, 'r') as f: stats = json.load(f) assert stats['files'] == {} assert stats["overall"]["loc"] == 0 def test_filelist_scan_malformed_json(tmp_path): """Test handling of malformed JSON file""" malformed_file = tmp_path / "malformed.json" with open(malformed_file, 'w') as f: f.write("{invalid json content") with pytest.raises(json.JSONDecodeError): run_modernmetric_with_files(str(malformed_file), tmp_path) def test_filelist_scan_missing_file(): """Test handling of missing input file""" with pytest.raises(FileNotFoundError): run_modernmetric_with_files("/nonexistent/input.json", Path("/tmp")) def test_filelist_scan_relative_paths(tmp_path): """Test handling of relative and absolute paths""" curr_dir = os.path.dirname(os.path.abspath(__file__)) files = { "files": [ "./relative/path/file.c", "../parent/path/file.cpp", str(Path(curr_dir) / "existing_file.py") ] } input_file = create_temp_json_file(files, tmp_path) _, output_file = run_modernmetric_with_files(input_file, tmp_path) with open(output_file, 'r') as f: stats = json.load(f) assert stats['files'] == {} # No files should be processed as they don't exist assert stats["overall"]["loc"] == 0

You'll need to:

Add import pytest at the top of the file if not already present

Ensure the test framework supports the tmp_path fixture (most recent pytest versions do)

Create appropriate test files in the testfiles directory if they don't exist

Update any existing error handling in the main modernmetric code if it doesn't properly handle these error cases

sourcery-ai · 2024-12-13T15:36:55Z

test/test_self_scan.py

+    assert files is not None
+    assert files["../testfiles/test.c"]["loc"] == 25
+    assert files["../testfiles/test.c"]["cyclomatic_complexity"] == 0


suggestion (testing): Test assertions could be more specific and descriptive

Consider using more specific assertions with meaningful error messages. For example: assert len(files) > 0, 'Files list should not be empty' and verify the exact number of expected files. Also, the cyclomatic complexity of 0 seems suspicious - is this the expected value?

Suggested implementation:

files = stats['files'] assert files is not None, "Files dictionary should not be None" assert len(files) == 1, "Expected exactly 1 file in analysis" test_file = files["../testfiles/test.c"] assert test_file is not None, "Test file '../testfiles/test.c' should be present in results" assert test_file["loc"] == 25, "Test file should have exactly 25 lines of code" # TODO: Verify if cyclomatic complexity of 0 is correct assert test_file["cyclomatic_complexity"] >= 1, "Cyclomatic complexity should be at least 1 for any non-empty file" assert stats["overall"]["loc"] == 178, "Overall LOC count should be 178"

Note: I've added a comment about cyclomatic complexity and changed the assertion to expect at least 1, since a value of 0 is suspicious for any non-empty file. The developer should:

Verify the expected cyclomatic complexity value for test.c

Update the assertion with the correct expected value once verified

Consider adding more assertions for other metrics if they are available in the stats output

aylusltd · 2024-12-13T19:53:50Z

modernmetric/license.py

@@ -3,7 +3,7 @@
 requestx = httpx.Client(http2=True, timeout=None)


-def report(identifier: str | int, product: str, die: bool = False):
+def report(identifier: "str | int", product: str, die: bool = False):


do this as a union imported from typing

aylusltd · 2024-12-13T19:56:20Z

modernmetric/__main__.py

@@ -147,7 +147,7 @@ def ArgParser(custom_args=None):
 # e.g. ["--file=path/to/filelist.json"]


-def main(custom_args=None, license_identifier: str | int = None):
+def main(custom_args=None, license_identifier: "str | int" = None):


use a union

aylusltd · 2024-12-13T19:57:13Z

.github/workflows/test.yml

@@ -39,11 +39,5 @@ jobs:
          python -m pip install --upgrade pip
          pip install -r requirements.txt

-      - name: Run Pytest
+      - name: Run Pytest with coverage


aren't you running pytest twice

Not exactly. test.yml runs PyTest 8 times. Mac and Linux x 4 Python versions. I moved Codecov out, so it doesn't upload the file 8 times, but it still needs to run pytest once to make the coverage file

Cache removed and filelist unit test

0bf4c0d

sourcery-ai bot reviewed Dec 13, 2024

View reviewed changes

SeanTConrad added 15 commits December 13, 2024 10:52

test print

c20e9b1

more debug prints

a79620f

operand support for python 3.8

9daab7a

another 3.8 fix

6230ddb

more debugging help

c97b900

more prints

c591b8d

more prints

092c786

something is broken

75d2919

maybe

75c1604

This shoud do it

5c81e0a

actually run coverage

9c41ce0

correct name

89894d2

moved codecov to run just once

6350c2c

codecov optimiztatons

0780d87

name

a82ac2a

SeanTConrad requested a review from aylusltd December 13, 2024 19:47

aylusltd reviewed Dec 13, 2024

View reviewed changes

SeanTConrad and others added 4 commits December 13, 2024 15:30

pr crs

42335a9

Merge branch 'master' into additional_unit_test

82c1c70

pr crs

30cbe08

spaces the same

7157e9a

aylusltd approved these changes Dec 17, 2024

View reviewed changes

aylusltd merged commit 2d11faf into master Dec 17, 2024
13 checks passed

aylusltd deleted the additional_unit_test branch December 17, 2024 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache removed and filelist unit test #32

Cache removed and filelist unit test #32

SeanTConrad commented Dec 13, 2024 •

edited

Loading

sourcery-ai bot commented Dec 13, 2024 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

sourcery-ai bot Dec 13, 2024

sourcery-ai bot Dec 13, 2024

aylusltd Dec 13, 2024

SeanTConrad Dec 13, 2024

aylusltd Dec 13, 2024

aylusltd Dec 13, 2024

SeanTConrad Dec 13, 2024 •

edited

Loading

Cache removed and filelist unit test #32

Cache removed and filelist unit test #32

Conversation

SeanTConrad commented Dec 13, 2024 • edited Loading

Summary by Sourcery

sourcery-ai bot commented Dec 13, 2024 • edited Loading

Reviewer's Guide by Sourcery

Class diagram for ArgParser changes

Class diagram for main function changes

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot Dec 13, 2024

Choose a reason for hiding this comment

sourcery-ai bot Dec 13, 2024

Choose a reason for hiding this comment

aylusltd Dec 13, 2024

Choose a reason for hiding this comment

SeanTConrad Dec 13, 2024

Choose a reason for hiding this comment

aylusltd Dec 13, 2024

Choose a reason for hiding this comment

aylusltd Dec 13, 2024

Choose a reason for hiding this comment

SeanTConrad Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

SeanTConrad commented Dec 13, 2024 •

edited

Loading

sourcery-ai bot commented Dec 13, 2024 •

edited

Loading

SeanTConrad Dec 13, 2024 •

edited

Loading