Skip to content

chore: fixtures: after delete() wait to verify deleted #1784

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 22, 2022
Merged

chore: fixtures: after delete() wait to verify deleted #1784

merged 1 commit into from
Jul 22, 2022

Conversation

JohnVillalovos
Copy link
Member

In our fixtures that create:

  • groups
  • project merge requests
  • projects
  • users

They delete the created objects after use. Now wait to ensure the
objects are deleted before continuing as having unexpected objects
existing can impact some of our tests.

@JohnVillalovos JohnVillalovos requested a review from nejch December 29, 2021 05:22
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/sidekiq branch 2 times, most recently from d684c74 to b0b137a Compare January 4, 2022 07:06
@JohnVillalovos JohnVillalovos marked this pull request as draft February 12, 2022 16:53
@JohnVillalovos JohnVillalovos marked this pull request as ready for review June 30, 2022 01:43
@JohnVillalovos
Copy link
Member Author

Example of error in tests that this is trying to fix:
https://github.com/python-gitlab/python-gitlab/runs/7123034880?check_suite_focus=true

        group1 = gl.groups.create({"name": "group1", "path": "group1"})
        group2 = gl.groups.create({"name": "group2", "path": "group2"})
        p_id = gl.groups.list(search="group2")[0].id
        group3 = gl.groups.create({"name": "group3", "path": "group3", "parent_id": p_id})
        group4 = gl.groups.create({"name": "group4", "path": "group4"})
>       assert len(gl.groups.list()) == 4
E       assert 5 == 4
E        +  where 5 = len([<Group id:35 name:group1>, <Group id:36 name:group2>, <Group id:37 name:group3>, <Group id:38 name:group4>, <Group id:6 name:test-group-45bec8b04d964c3fafd743afcb68f528>])
E        +    where [<Group id:35 name:group1>, <Group id:36 name:group2>, <Group id:37 name:group3>, <Group id:38 name:group4>, <Group id:6 name:test-group-45bec8b04d964c3fafd743afcb68f528>] = <bound method ListMixin.list of <gitlab.v4.objects.groups.GroupManager object at 0x7f1e1b486ce0>>()
E        +      where <bound method ListMixin.list of <gitlab.v4.objects.groups.GroupManager object at 0x7f1e1b486ce0>> = <gitlab.v4.objects.groups.GroupManager object at 0x7f1e1b486ce0>.list
E        +        where <gitlab.v4.objects.groups.GroupManager object at 0x7f1e1b486ce0> = <gitlab.client.Gitlab object at 0x7f1e1b485d80>.groups
tests/functional/api/test_groups.py:31: AssertionError

@JohnVillalovos
Copy link
Member Author

@nejch FYI: I'd like to get this in before trying to get #1778 in. Also #1785 should get in too.

@nejch
Copy link
Member

nejch commented Jul 1, 2022

@nejch FYI: I'd like to get this in before trying to get #1778 in. Also #1785 should get in too.

Thanks @JohnVillalovos I'll try to get back into the functional testing topic a bit.. I think we should make this reliable for sure, although it also shows we have some poor assertions in those legacy tests there.

IMO we should never be asserting len() on any lists, just asserting on membership of those objects in the lists. That way our tests also wouldn't be as affected by async gitlab operations. E.g. if we create/delete a resource we shouldn't care if there are 9 or 10 of them on the server, only if that one exists or not :) Anyway, just some ramblings before I forget this, probably belongs in a separate issue :D

nejch
nejch requested changes Jul 3, 2022
View reviewed changes
@JohnVillalovos JohnVillalovos marked this pull request as draft July 3, 2022 19:30
@nejch nejch mentioned this pull request Jul 3, 2022
Merged
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/sidekiq branch 6 times, most recently from 80122a7 to 7a86c8d Compare July 4, 2022 02:15
@JohnVillalovos JohnVillalovos marked this pull request as ready for review July 4, 2022 02:18
@JohnVillalovos JohnVillalovos requested a review from nejch July 4, 2022 02:18
nejch
nejch reviewed Jul 20, 2022
View reviewed changes
nejch
nejch reviewed Jul 20, 2022
View reviewed changes
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/sidekiq branch 2 times, most recently from 1dcf0f4 to b8d3aa5 Compare July 20, 2022 22:18
@JohnVillalovos JohnVillalovos requested a review from nejch July 20, 2022 22:29
@nejch
Copy link
Member

nejch commented Jul 21, 2022

Thanks again @JohnVillalovos, just a few more random thoughts as I like that we're potentially even reducing the codebase here :)

nejch
nejch reviewed Jul 21, 2022
View reviewed changes
@JohnVillalovos
chore: fixtures: after delete() wait to verify deleted
1f73b6b 
In our fixtures that create:
  - groups
  - project merge requests
  - projects
  - users

They delete the created objects after use. Now wait to ensure the
objects are deleted before continuing as having unexpected objects
existing can impact some of our tests.
@JohnVillalovos JohnVillalovos requested a review from nejch July 22, 2022 01:03
@nejch nejch merged commit 916b1db into main Jul 22, 2022
@nejch nejch deleted the jlvillal/sidekiq branch July 22, 2022 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nejch nejch Awaiting requested review from nejch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JohnVillalovos @nejch