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

fix: len(None) bug #643

Merged
merged 5 commits into from
Feb 20, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions codecov_cli/helpers/versioning_systems.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,27 @@

from codecov_cli.fallbacks import FallbackFieldEnum
from codecov_cli.helpers.git import parse_git_service, parse_slug
from abc import ABC, abstractmethod

logger = logging.getLogger("codecovcli")


class VersioningSystemInterface(object):
class VersioningSystemInterface(ABC):
def __repr__(self) -> str:
return str(type(self))

@abstractmethod
def get_fallback_value(self, fallback_field: FallbackFieldEnum) -> t.Optional[str]:
pass

@abstractmethod
def get_network_root(self) -> t.Optional[Path]:
pass

def list_relevant_files(self, directory: t.Optional[Path] = None) -> t.List[str]:
@abstractmethod
def list_relevant_files(
self, directory: t.Optional[Path] = None
) -> t.Optional[t.List[str]]:
pass


Expand Down Expand Up @@ -119,8 +125,8 @@ def get_network_root(self):
return Path(p.stdout.decode().rstrip())
return None

def list_relevant_files(self, root_folder: t.Optional[Path] = None) -> t.List[str]:
dir_to_use = root_folder or self.get_network_root()
def list_relevant_files(self, directory: t.Optional[Path] = None) -> t.List[str]:
dir_to_use = directory or self.get_network_root()
if dir_to_use is None:
raise ValueError("Can't determine root folder")

Expand All @@ -145,3 +151,9 @@ def is_available(cls):

def get_network_root(self):
return Path.cwd()

def get_fallback_value(self, fallback_field: FallbackFieldEnum):
return None

def list_relevant_files(self, directory: t.Optional[Path] = None) -> t.List[str]:
return []
2 changes: 1 addition & 1 deletion codecov_cli/services/upload/network_finder.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def __init__(
def find_files(self, ignore_filters=False) -> typing.List[str]:

Choose a reason for hiding this comment

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

Is mypy run on this project?
If files is None (as described in the PR description), your changed code surely results in returning None, in which case this annotation is wrong and there's a good chance another part of the codebase could mess up when expecting a list.

Copy link

@Dreamsorcerer Dreamsorcerer Feb 20, 2025

Choose a reason for hiding this comment

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

OK, I see the root problem (which confirms to me that this is not being checked by mypy, as it should have errored here).

The method is defined on the base class, but it's not an abstract method. So, if inherited (or instantiated directly), this returns None, even though the annotation says it returns a list (which is a clear type error that would be caught by any type checker):

def list_relevant_files(self, directory: t.Optional[Path] = None) -> t.List[str]:
pass

I think the correct fix is to either replace that pass with a return [], so it actually does the correct behaviour, or to turn those methods into abstract methods and require the subclass to define them correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback, I've implemented the ABC / abstractmethod suggestion.

files = self.versioning_system.list_relevant_files(self.network_root_folder)

if not ignore_filters:
if files and not ignore_filters:
if self.network_filter:
files = [file for file in files if file.startswith(self.network_filter)]
if self.network_prefix:
Expand Down
2 changes: 1 addition & 1 deletion codecov_cli/services/upload/upload_collector.py
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ def generate_upload_data(
f"Found {len(report_files)} {report_type.value} files to report"
)
logger.debug(
f"Found {len(network)} network files to report, ({len(unfiltered_network)} without filtering)"
f"Found {len(network) if network else None} network files to report, ({len(unfiltered_network) if network else None} without filtering)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
f"Found {len(network) if network else None} network files to report, ({len(unfiltered_network) if network else None} without filtering)"
f"Found {len(network) if network else 0} network files to report, ({len(unfiltered_network) if network else 0} without filtering)"

)
if not report_files:
if report_type == ReportType.TEST_RESULTS:
Expand Down
7 changes: 7 additions & 0 deletions tests/factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from codecov_cli.helpers.ci_adapters.base import CIAdapterBase
from codecov_cli.helpers.versioning_systems import VersioningSystemInterface
from codecov_cli.runners.types import LabelAnalysisRunnerInterface
from pathlib import Path


class FakeProvider(CIAdapterBase):
Expand Down Expand Up @@ -73,6 +74,12 @@ def __init__(self, values_dict: Optional[dict] = None):
def get_fallback_value(self, fallback_field: FallbackFieldEnum) -> Optional[str]:
return self.values_dict[fallback_field]

def get_network_root(self) -> Optional[Path]:
return None

def list_relevant_files(self, directory: Optional[Path] = None) -> List[str]:
return []


class FakeRunner(LabelAnalysisRunnerInterface):
dry_run_runner_options = ["--labels"]
Expand Down
32 changes: 31 additions & 1 deletion tests/services/upload/test_upload_collector.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
from pathlib import Path
from unittest.mock import patch

from codecov_cli.helpers.versioning_systems import GitVersioningSystem
from codecov_cli.helpers.versioning_systems import (
GitVersioningSystem,
NoVersioningSystem,
)
from codecov_cli.services.upload.file_finder import FileFinder
from codecov_cli.services.upload.network_finder import NetworkFinder
from codecov_cli.services.upload.upload_collector import UploadCollector
Expand Down Expand Up @@ -174,3 +177,30 @@ def test_generate_upload_data(tmp_path):

for file in expected:
assert file in res.files


@patch("codecov_cli.services.upload.upload_collector.logger")
@patch.object(GitVersioningSystem, "get_network_root", return_value=None)
def test_generate_upload_data_with_none_network(
mock_get_network_root, mock_logger, tmp_path
):
(tmp_path / "coverage.xml").touch()

file_finder = FileFinder(tmp_path)
network_finder = NetworkFinder(NoVersioningSystem(), None, None, None)

collector = UploadCollector([], network_finder, file_finder, {})

res = collector.generate_upload_data()

mock_logger.debug.assert_any_call("Collecting relevant files")
mock_logger.debug.assert_any_call(
"Found None network files to report, (None without filtering)"
)

mock_logger.info.assert_any_call("Found 1 coverage files to report")
mock_logger.info.assert_any_call("> {}".format(tmp_path / "coverage.xml"))

assert res.network == []
assert len(res.files) == 1
assert len(res.file_fixes) == 0