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

Packages/test performance #921

Merged
merged 3 commits into from
May 23, 2023
Merged

Conversation

abandoned-prototype
Copy link
Collaborator

Status

Ready for review

Description of Changes

Changes proposed in this pull request:

  • For some reason the mockdata used in the tests not only contains a lot of data, but it was freshly inserted into the database for every test that used it.
    • Now we will load the mockdata into the in-memory database only once (via the fixture with scope session). This makes tests significantly faster.
    • Adding an additional department to the mockdata broke some tests which had to be fixed.
    • Changing anything in the mockdata brakes tests because some of the data is assigned pseudo-randomly (random with fixed seed), so a test might fail because an officer used to not have a link in the profile but does after the change. I fixed the failing tests in a way that this does not happen again.

Notes for Deployment

Changes tests only

Tests and linting

  • I have rebased my changes on current develop

  • pytests pass in the development environment on my local machine

  • pre-commit checks pass

Copy link
Collaborator

@michplunkett michplunkett left a comment

Choose a reason for hiding this comment

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

Looks good! I just have some formatting suggestions, but they aren't blockers.

Comment on lines 53 to 54
DEPARTMENT_TWO_NAME = "Chicago Police Department"
DEPARTMENT_EMPTY_NAME = "Empty Police Department"
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔬 I think DEPARTMENT_NAME_XXXX makes more sense as a naming motif.

DEPARTMENT_NAME -> DEPARTMENT_NAME_SPRINGFIELD
DEPARTMENT_TWO_NAME -> DEPARTMENT_NAME_CHICAGO
DEPARTMENT_EMPTY_NAME -> DEPARTMENT_NAME_EMPTY.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed it slightly, hopefully make it a little more sense. But I want to make it clearer which department is considered the default one.

Comment on lines 53 to 54
DEPARTMENT_TWO_NAME = "Chicago Police Department"
DEPARTMENT_EMPTY_NAME = "Empty Police Department"
Copy link
Collaborator

Choose a reason for hiding this comment

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

When I see "empty" in the name, I presume it's an empty string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Changed it to DEPARTMENT_WITHOUT_OFFICERS.

@@ -310,15 +339,18 @@ def add_mockdata(session):

test_images = [
models.Image(
filepath="/static/images/test_cop{}.png".format(x + 1), department_id=1
filepath="/static/images/test_cop{}.png".format(x + 1),
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔬 An f-string feels like a more intuitive way to write this, imo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Strongly agree, this is from old code. Changed this one. And maybe we can use https://github.com/ikamensh/flynt or something next to get rid of all of the .format.

Comment on lines 398 to 400
for officer in officers_dept2[
: math.ceil(len(officers_dept2) * assignment_ratio)
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔬 The syntax here doesn't feel incredibly intuitive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tried to make it a little better, by extracting "the math" to a variable and adding a comment.

Comment on lines 52 to 54
DEPARTMENT_NAME = "Springfield Police Department"
DEPARTMENT_TWO_NAME = "Chicago Police Department"
DEPARTMENT_EMPTY_NAME = "Empty Police Department"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since these are paired to specific short names, I think it may be valuable to put them in a dictionary.

DEPARTMENT_KEY_SPD = "SPD"
DEPARTMENT_KEY_CPD = "CPD"
DEPARTMENT_KEY_EPD = "EPD"

DEPARTMENTS = {
    DEPARTMENT_KEY_SPD: "Springfield Police Department",
    DEPARTMENT_KEY_CPD: "Empty Police Department",
    DEPARTMENT_KEY_EPD: "Chicago Police Department"
}

You can use the key as the short name (lower or upper case, whateva) and the value as the long name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

On second thought, tuples would work as well.

DEPARTMENT_SPD = ("SPD", "Springfield Police Department")
DEPARTMENT_CPD = ("CPD", "Chicago Police Department")
DEPARTMENT_EPD = ("EPD", "Empty Police Department")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The department short name does not seem to matter much for tests (they are not used anywhere other than the initial creation) so I decided against giving each their own variable.

@abandoned-prototype abandoned-prototype merged commit c919c6b into develop May 23, 2023
@abandoned-prototype abandoned-prototype deleted the packages/test-performance branch May 23, 2023 05:02
sea-kelp pushed a commit to OrcaCollective/OpenOversight that referenced this pull request Sep 6, 2023
* Make sure adding mockdata is only run once.

* Fix tests.
sea-kelp pushed a commit to OrcaCollective/OpenOversight that referenced this pull request Sep 25, 2023
* Make sure adding mockdata is only run once.

* Fix tests.
sea-kelp pushed a commit to OrcaCollective/OpenOversight that referenced this pull request Oct 5, 2023
* Make sure adding mockdata is only run once.

* Fix tests.
AetherUnbound pushed a commit to OrcaCollective/OpenOversight that referenced this pull request Oct 9, 2023
* Make sure adding mockdata is only run once.

* Fix tests.
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