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

Test metrics #140

Merged
merged 21 commits into from
Mar 18, 2021
Merged

Test metrics #140

merged 21 commits into from
Mar 18, 2021

Conversation

JFer11
Copy link
Contributor

@JFer11 JFer11 commented Mar 1, 2021

In this branch, tests for the following endpoints were added:
/metrics/cameras/{camera_id}/heatmap
/metrics/cameras/social-distancing/live
/metrics/cameras/social-distancing/hourly
/metrics/cameras/social-distancing/daily
/metrics/cameras/social-distancing/weekly
/metrics/cameras/face-mask-detections/live
/metrics/cameras/face-mask-detections/hourly
/metrics/cameras/face-mask-detections/daily
/metrics/cameras/face-mask-detections/weekly
/metrics/areas/social-distancing/live
/metrics/areas/social-distancing/hourly
/metrics/areas/social-distancing/daily
/metrics/areas/social-distancing/weekly
/metrics/areas/face-mask-detections/live
/metrics/areas/face-mask-detections/hourly
/metrics/areas/face-mask-detections/daily
/metrics/areas/face-mask-detections/weekly
/metrics/areas/occupancy/live
/metrics/areas/occupancy/hourly
/metrics/areas/occupancy/daily
/metrics/areas/occupancy/weekly

Note: there are some comments with the tag#TODO that should be ignored for now.

Copy link
Contributor

@mats-claassen mats-claassen left a comment

Choose a reason for hiding this comment

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

api/tests/data/detections_heatmap_2020-09-19_EXAMPLE.npy is an empty file. Maybe you want to remove it?

api/tests/utils/fixtures_tests.py Outdated Show resolved Hide resolved
api/tests/utils/fixtures_tests.py Outdated Show resolved Hide resolved
api/tests/app/test_camera_metrics.py Outdated Show resolved Hide resolved
api/tests/app/test_camera_metrics.py Outdated Show resolved Hide resolved
api/tests/app/test_camera_metrics.py Show resolved Hide resolved
api/tests/app/test_camera_metrics.py Outdated Show resolved Hide resolved
api/tests/app/test_camera_metrics.py Outdated Show resolved Hide resolved
api/tests/app/test_camera_metrics.py Outdated Show resolved Hide resolved
api/tests/app/test_camera_metrics.py Outdated Show resolved Hide resolved
api/tests/app/test_camera_metrics.py Outdated Show resolved Hide resolved
api/tests/app/test_camera_metrics.py Outdated Show resolved Hide resolved
api/tests/app/test_camera_metrics.py Outdated Show resolved Hide resolved
api/tests/app/test_camera_metrics.py Outdated Show resolved Hide resolved
@@ -45,8 +45,8 @@ def get_area_occupancy_daily_report(areas: str = "",
def get_area_occupancy_weekly_report(
areas: str = "",
weeks: int = Query(0),
from_date: date = Query((date.today() - timedelta(days=date.today().weekday(), weeks=4)).isoformat()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing this?

Copy link
Contributor Author

@JFer11 JFer11 Mar 17, 2021

Choose a reason for hiding this comment

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

Because this causes several bugs.

First, when you do not send neither from_date nor the to_date as a query parameter, default values will take place and will be sent to the corresponding report function. However, in both cases, a string will be sent, not a date. (Important: .isoformat() returns an string).
So, for example, when get_weekly_metric is reached and we have to set a week_span:

        if number_of_days > 0:
            # Separate weeks in range taking a number of weeks ago, considering the week ended yesterday
            date_range = pd.date_range(end=date.today() - timedelta(days=1), periods=number_of_days)
            start_dates = date_range[0::7]
            end_dates = date_range[6::7]
            week_span = list(zip(start_dates, end_dates))
        elif isinstance(from_date, date) and isinstance(to_date, date):
            # Separate weeks in range considering the week starts on Monday
            date_range = pd.date_range(start=from_date, end=to_date)
            week_span = list(parse_date_range(date_range))
        else:
            week_span = []

Given range of dates are ok, but are both strings, so will never enter elif isinstance(from_date, date) and isinstance(to_date, date): and week_span will be an empty array, and this, should not be like that. As a result, no data will be returned on the endpoint, regardless of whether or not there is.

Secondly, there was another issue that appears when you submit only one the two dates a let the other one be the default value. As we explained before, default values will be string, while submitted dates are type: "date". So when the code reaches:

def validate_dates(from_date: date, to_date: date):
    if from_date > to_date:
        raise HTTPException(
            status_code=status.HTTP_400_BAD_REQUEST,
            detail=bad_request_serializer(
                "Invalid range of dates",
                error_type="from_date doesn't come before to_date",
                loc=["query", "from_date"]
            )
        )

An exception will be thrown because we cannot compare a string with a date.

@renzodgc renzodgc merged commit d63224f into galliot-us:master Mar 18, 2021
@renzodgc renzodgc deleted the test_metrics branch March 18, 2021 18:44
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.

3 participants