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

[TechDebt] Fix test coverage Tasking::construct_order_parameters #680

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

andher1802
Copy link
Contributor

See changelog.

Checklist:

  • Test coverage
  • Version bump
  • Changelog update

@andher1802 andher1802 requested review from up42-eva and a team as code owners October 9, 2024 09:08
@dustgalactic dustgalactic self-requested a review October 16, 2024 09:37
acquisition_end="2022-11-10",
geometry=mock_search_parameters["intersects"],
class TestTasking:
@pytest.fixture(autouse=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

autouse is not needed

@@ -14,6 +14,16 @@

logger = utils.get_logger(__name__)

Geometry = Union[
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we use catalog.Geometry?

Copy link
Contributor Author

@andher1802 andher1802 Oct 16, 2024

Choose a reason for hiding this comment

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

they are not the same. We can unify both, but I haven't tested if this has an impact.

# catalog.py
Geometry = Union[
    dict,
    geojson.Feature,
    geojson.FeatureCollection,
    list,
    geopandas.GeoDataFrame,
    geom.Polygon,
]

pyproject.toml Outdated
@@ -1,6 +1,6 @@
[tool.poetry]
name = "up42-py"
version = "2.1.0"
version = "2.2.0a1"
Copy link
Contributor

Choose a reason for hiding this comment

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

It's 2.1.1a1

up42/tasking.py Outdated
logger.info("See `tasking.get_data_product_schema(data_product_id)` for more detail on the parameter options.")
missing_params = {param: order_parameters["params"].get(param) for param in schema["required"]}
order_parameters["params"].update(missing_params)

geometry = utils.any_vector_to_fc(vector=geometry)
assert isinstance(order_parameters["params"], dict)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed

up42/tasking.py Outdated
}
schema = self.get_data_product_schema(data_product_id)
params: dict[str, Any] = {param: None for param in schema["required"]}
params["displayName"] = name
Copy link
Contributor

Choose a reason for hiding this comment

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

params |= {
            "displayName": name,
            "acquisitionStart": utils.format_time(acquisition_start),
            "acquisitionEnd": utils.format_time(acquisition_end, set_end_of_day=True),
        }

params["displayName"] = name
params["acquisitionStart"] = utils.format_time(acquisition_start)
params["acquisitionEnd"] = utils.format_time(acquisition_end, set_end_of_day=True)
order_parameters: order.OrderParams = {"dataProduct": data_product_id, "params": params}
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need typing hint here?

Copy link
Contributor Author

@andher1802 andher1802 Oct 16, 2024

Choose a reason for hiding this comment

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

If we don't add type, wouldn't be a dict type? pre-commit complains about it

assert isinstance(order_parameters, dict)
assert list(order_parameters.keys()) == ["dataProduct", "params"]
assert order_parameters["params"]["acquisitionMode"] is None
def test_construct_order_parameters(
Copy link
Contributor

Choose a reason for hiding this comment

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

test_should_construct_order_parameters

)
order_parameters = tasking_obj.construct_order_parameters(
data_product_id=constants.DATA_PRODUCT_ID,
name="some-name",
Copy link
Contributor

Choose a reason for hiding this comment

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

let's extract this to a variable or constant since it's longer than 5 symbols

self,
requests_mock: req_mock.Mocker,
tasking_obj: tasking.Tasking,
input_geometry: Optional[tasking.Geometry],
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not optional

Comment on lines 104 to 105
"acquisitionStart": "2014-01-01T00:00:00Z",
"acquisitionEnd": "2022-12-31T23:59:59Z",
Copy link
Contributor

Choose a reason for hiding this comment

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

These two values should not be hard coded in the test, to be extracted to constants

Copy link

sonarcloud bot commented Oct 16, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants