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: Remove duplicate names for resolvers and tests #527

Merged
merged 2 commits into from
May 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
18 changes: 0 additions & 18 deletions api/internal/tests/views/test_coverage_viewset.py
Original file line number Diff line number Diff line change
Expand Up @@ -333,24 +333,6 @@ def test_tree_not_found_for_components(
res = self._tree(components="Does_not_exist")
assert res.status_code == 404

@patch("shared.reports.api_report_service.build_report_from_commit")
@patch("services.components.commit_components")
def test_tree_not_found_for_components(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test looked to be an exact copy of the one above it

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch

self, commit_components_mock, build_report_from_commit
):
commit_components_mock.return_value = [
Component.from_dict(
{
"component_id": "c1",
"name": "ComponentOne",
"paths": ["dne.py"],
}
),
]
build_report_from_commit.return_value = sample_report()
res = self._tree(components="Does_not_exist")
assert res.status_code == 404

@patch("shared.reports.api_report_service.build_report_from_commit")
def test_tree_no_data_for_flags(self, build_report_from_commit):
build_report_from_commit.return_value = sample_report()
Expand Down
2 changes: 1 addition & 1 deletion codecov_auth/commands/owner/tests/test_owner.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ def test_save_terms_agreement_delegate_to_interactor(self, interactor_mock):
interactor_mock.assert_called_once_with(input_dict)

@patch("codecov_auth.commands.owner.owner.StartTrialInteractor.execute")
def test_cancel_trial_delegate_to_interactor(self, interactor_mock):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For all the resolvers, I just named them what the property was

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes weren't necessary though; just a cleanliness thing

def test_start_trial_delegate_to_interactor(self, interactor_mock):
org_username = "random_org"
self.command.start_trial(org_username=org_username)
interactor_mock.assert_called_once_with(org_username=org_username)
Expand Down
5 changes: 3 additions & 2 deletions codecov_auth/tests/unit/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,12 @@ def test_current_user_part_of_org_when_user_is_owner():
def test_current_user_part_of_org_when_user_doesnt_have_org():
org = OwnerFactory()
current_user = OwnerFactory(organizations=None)
assert current_user_part_of_org(current_user, current_user) is False
current_user.save()
assert current_user_part_of_org(current_user, org) is False


@pytest.mark.django_db
def test_current_user_part_of_org_when_user_doesnt_have_org():
def test_current_user_part_of_org_when_user_has_org():
org = OwnerFactory()
current_user = OwnerFactory(organizations=[org.ownerid])
current_user.save()
Expand Down
9 changes: 8 additions & 1 deletion codecov_auth/tests/unit/views/test_gitlab.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@ async def helper_list_teams_func(*args, **kwargs):
as_tuple=mocker.MagicMock(return_value=("a", "b"))
),
)

session = client.session
session["gitlab_oauth_state"] = "abc"
session.save()

url = reverse("gitlab-login")
mock_redis.setex("oauth-state-abc", 300, "http://localhost:3000/gl")
res = client.get(url, {"code": "aaaaaaa", "state": "abc"})
Expand All @@ -92,7 +97,9 @@ async def helper_list_teams_func(*args, **kwargs):
assert encryptor.decode(owner.oauth_token) == f"{access_token}: :{refresh_token}"


def test_get_gitlab_already_with_code(client, mocker, db, settings, mock_redis):
def test_get_gitlab_already_with_code_no_session(
client, mocker, db, settings, mock_redis
):
settings.GITLAB_CLIENT_ID = (
"testfiuozujcfo5kxgigugr5x3xxx2ukgyandp16x6w566uits7f32crzl4yvmth"
)
Expand Down
31 changes: 1 addition & 30 deletions graphql_api/tests/test_branch.py
Original file line number Diff line number Diff line change
Expand Up @@ -628,36 +628,7 @@ def test_fetch_path_contents_unknown_path(
}

@patch("services.report.build_report_from_commit")
def test_fetch_path_contents_unknown_flags(self, report_mock):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this looked to be a duplicate of the below test that was never updated

report_mock.return_value = MockReport()

data = self.gql_request(
query_files,
variables={
"org": self.org.username,
"repo": self.repo.name,
"branch": self.branch.name,
"path": "",
"filters": {"flags": ["test-123"]},
},
)
assert data == {
"owner": {
"repository": {
"branch": {
"head": {
"pathContents": {
"__typename": "UnknownFlags",
"message": "No coverage with chosen flags",
}
}
}
}
}
}

@patch("services.report.build_report_from_commit")
def test_fetch_path_contents_unknown_flags(self, report_mock):
def test_fetch_path_contents_unknown_flags_no_flags(self, report_mock):
report_mock.return_value = MockNoFlagsReport()

data = self.gql_request(
Expand Down
62 changes: 0 additions & 62 deletions graphql_api/tests/test_pull_comparison.py
Original file line number Diff line number Diff line change
Expand Up @@ -979,68 +979,6 @@ def test_pull_comparison_no_comparison(self, compute_comparisons_mock):

compute_comparisons_mock.assert_called_once

def test_pull_comparison_missing_head_report(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This and the following test are using deprecated patterns

self.head_report.side_effect = comparison.MissingComparisonReport(
"Missing head report"
)

query = """
pullId
compareWithBase {
... on Comparison {
state
impactedFilesDeprecated {
headName
}
}
}
"""

res = self.gql_request(
base_query % (self.repository.name, self.pull.pullid, query),
owner=self.owner,
with_errors=True,
)
assert res["errors"] is not None
assert res["errors"][0]["message"] == "Missing head report"
assert (
res["data"]["me"]["owner"]["repository"]["pull"]["compareWithBase"][
"impactedFilesDeprecated"
]
is None
)

def test_pull_comparison_missing_base_report(self):
self.base_report.side_effect = comparison.MissingComparisonReport(
"Missing base report"
)

query = """
pullId
compareWithBase {
... on Comparison {
state
impactedFilesDeprecated {
headName
}
}
}
"""

res = self.gql_request(
base_query % (self.repository.name, self.pull.pullid, query),
owner=self.owner,
with_errors=True,
)
assert res["errors"] is not None
assert res["errors"][0]["message"] == "Missing base report"
assert (
res["data"]["me"]["owner"]["repository"]["pull"]["compareWithBase"][
"impactedFilesDeprecated"
]
is None
)

def test_pull_comparison_missing_when_commit_comparison_state_is_errored(self):
self.commit_comparison.state = CommitComparison.CommitComparisonStates.ERROR
self.commit_comparison.save()
Expand Down
16 changes: 9 additions & 7 deletions graphql_api/types/bundle_analysis/base.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import List, Mapping
from typing import List, Mapping, Optional

from ariadne import ObjectType

Expand Down Expand Up @@ -34,20 +34,20 @@ def resolve_bundle_load_time(bundle_data: BundleData, info) -> BundleLoadTime:


@bundle_module_bindable.field("name")
def resolve_name(bundle_module: ModuleReport, info) -> str:
def resolve_bundle_module_name(bundle_module: ModuleReport, info) -> str:
return bundle_module.name


@bundle_module_bindable.field("bundleData")
def resolve_bundle_data(bundle_module: ModuleReport, info) -> int:
def resolve_bundle_module_bundle_data(bundle_module: ModuleReport, info) -> int:
return BundleData(bundle_module.size_total)


# ============= Bundle Asset Bindable =============


@bundle_asset_bindable.field("name")
def resolve_name(bundle_asset: AssetReport, info) -> str:
def resolve_bundle_asset_name(bundle_asset: AssetReport, info) -> str:
return bundle_asset.name


Expand All @@ -62,7 +62,7 @@ def resolve_extension(bundle_asset: AssetReport, info) -> str:


@bundle_asset_bindable.field("bundleData")
def resolve_bundle_data(bundle_asset: AssetReport, info) -> BundleData:
def resolve_bundle_asset_bundle_data(bundle_asset: AssetReport, info) -> BundleData:
return BundleData(bundle_asset.size_total)


Expand All @@ -72,7 +72,9 @@ def resolve_modules(bundle_asset: AssetReport, info) -> List[ModuleReport]:


@bundle_asset_bindable.field("moduleExtensions")
def resolve_module_extensions(bundle_asset: AssetReport, info) -> List[str]:
def resolve_bundle_asset_module_extensions(
bundle_asset: AssetReport, info
) -> List[str]:
return bundle_asset.module_extensions


Expand Down Expand Up @@ -110,7 +112,7 @@ def resolve_module_count(bundle_report: BundleReport, info) -> int:
def resolve_assets(
bundle_report: BundleReport,
info,
filters: Mapping = None,
filters: Optional[Mapping] = None,
) -> List[AssetReport]:
extensions_filter = filters.get("moduleExtensions", None) if filters else None
return list(bundle_report.assets(extensions_filter))
Expand Down
20 changes: 13 additions & 7 deletions graphql_api/types/bundle_analysis/comparison.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,43 +35,49 @@ def resolve_bundle_analysis_comparison_result_type(obj, *_):


@bundle_analysis_comparison_bindable.field("sizeDelta")
def resolve_size_delta(bundles_analysis_comparison: BundleAnalysisComparison, info):
def resolve_ba_comparison_size_delta(
bundles_analysis_comparison: BundleAnalysisComparison, info
):
return bundles_analysis_comparison.size_delta


@bundle_analysis_comparison_bindable.field("sizeTotal")
def resolve_size_total(bundles_analysis_comparison: BundleAnalysisComparison, info):
def resolve_ba_comparison_size_total(
bundles_analysis_comparison: BundleAnalysisComparison, info
):
return bundles_analysis_comparison.size_total


@bundle_analysis_comparison_bindable.field("loadTimeDelta")
def resolve_load_time_delta(
def resolve_ba_comparison_load_time_delta(
bundles_analysis_comparison: BundleAnalysisComparison, info
):
return bundles_analysis_comparison.load_time_delta


@bundle_analysis_comparison_bindable.field("loadTimeTotal")
def resolve_load_time_total(
def resolve_ba_comparison_load_time_total(
bundles_analysis_comparison: BundleAnalysisComparison, info
):
return bundles_analysis_comparison.load_time_total


@bundle_analysis_comparison_bindable.field("bundles")
def resolve_bundles(bundles_analysis_comparison: BundleAnalysisComparison, info):
def resolve_ba_comparison_bundles(
bundles_analysis_comparison: BundleAnalysisComparison, info
):
return bundles_analysis_comparison.bundles


@bundle_analysis_comparison_bindable.field("bundleData")
def resolve_bundle_data(
def resolve_ba_comparison_bundle_data(
bundles_analysis_comparison: BundleAnalysisComparison, info
) -> BundleData:
return BundleData(bundles_analysis_comparison.size_total)


@bundle_analysis_comparison_bindable.field("bundleChange")
def resolve_bundle_delta(
def resolve_ba_comparison_bundle_delta(
bundles_analysis_comparison: BundleAnalysisComparison, info
) -> BundleData:
return BundleData(bundles_analysis_comparison.size_delta)
Expand Down
6 changes: 3 additions & 3 deletions graphql_api/types/comparison/comparison.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def resolve_state(comparison: ComparisonReport, info) -> str:
@comparison_bindable.field("impactedFilesDeprecated")
@convert_kwargs_to_snake_case
@sync_to_async
def resolve_impacted_files(
def resolve_impacted_files_deprecated(
comparison_report: ComparisonReport, info, filters=None
) -> List[ImpactedFile]:
command: CompareCommands = info.context["executor"].get_command("compare")
Expand Down Expand Up @@ -240,9 +240,9 @@ def resolve_flag_comparisons_count(comparison: ComparisonReport, info):
@comparison_bindable.field("hasDifferentNumberOfHeadAndBaseReports")
@sync_to_async
def resolve_has_different_number_of_head_and_base_reports(
comparison: ComparisonReport, info, **kwargs
comparison: ComparisonReport, info, **kwargs # type: ignore
) -> False:
# TODO: can we remove the need for `info.context["conmparison"]` here?
# TODO: can we remove the need for `info.context["comparison"]` here?
if "comparison" not in info.context:
return False
comparison: Comparison = info.context["comparison"]
Expand Down
4 changes: 2 additions & 2 deletions graphql_api/types/file/file.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def get_coverage_type(line_report):


@file_bindable.field("coverage")
def resolve_content(data, info):
def resolve_coverage(data, info):
file_report = data.get("file_report")

if not file_report:
Expand All @@ -42,7 +42,7 @@ def resolve_content(data, info):


@file_bindable.field("totals")
def resolve_content(data, info):
def resolve_totals(data, info):
file_report = data.get("file_report")
return file_report.totals if file_report else None

Expand Down
2 changes: 1 addition & 1 deletion graphql_api/types/me/me.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ def resolve_business_email(current_owner: Owner, _, **kwargs) -> Optional[str]:

@me_bindable.field("privateAccess")
@sync_to_async
def resolve_profile(owner: Owner, info) -> bool:
def resolve_private_access(owner: Owner, info) -> bool:
if owner.private_access is None:
return False
return owner.private_access
Expand Down
2 changes: 1 addition & 1 deletion graphql_api/types/plan/plan.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def resolve_plan_name(plan_service: PlanService, info) -> str:


@plan_bindable.field("value")
def resolve_plan_name(plan_service: PlanService, info) -> str:
def resolve_plan_name_as_value(plan_service: PlanService, info) -> str:
return plan_service.plan_name


Expand Down
2 changes: 1 addition & 1 deletion graphs/tests/test_badge_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -1303,7 +1303,7 @@ def test_commit_report_null(self, full_report_mock):
assert response.status_code == status.HTTP_200_OK

@patch("core.models.Commit.full_report", new_callable=PropertyMock)
def test_commit_report_null(self, full_report_mock):
def test_commit_report_no_flags(self, full_report_mock):
gh_owner = OwnerFactory(service="github")
repo = RepositoryFactory(
author=gh_owner, active=True, private=False, name="repo1"
Expand Down
2 changes: 1 addition & 1 deletion services/bundle_analysis.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ def size_total(self) -> int:
return self.asset.size

@cached_property
def modules(self) -> ModuleReport:
def modules(self) -> List[ModuleReport]:
return [ModuleReport(module) for module in self.asset.modules()]

@cached_property
Expand Down
6 changes: 3 additions & 3 deletions services/tests/test_components.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ def setUp(self):
self.comparison = Comparison(self.user, self.base_commit, self.head_commit)

@patch("services.comparison.Comparison.base_report", new_callable=PropertyMock)
def test_head_report(self, base_report_mock):
def test_base_report(self, base_report_mock):
report = sample_report()
base_report_mock.return_value = report

Expand All @@ -153,9 +153,9 @@ def test_head_report(self, base_report_mock):
}
)
component_comparison = ComponentComparison(self.comparison, component_go)
assert component_comparison.head_report.files == ["file_1.go"]
assert component_comparison.base_report.files == ["file_1.go"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test wasn't actually looking at base_report

assert (
component_comparison.head_report.totals.coverage
component_comparison.base_report.totals.coverage
== report.get("file_1.go").totals.coverage
)

Expand Down
Loading
Loading