@nejch - something has clearly changed in user_agent which was causing failures when we did snipper.user_agent_details()

I can't see exactly what value that has in our tests, I will add one as a separate test marked as skipped - as I can't see anything in the changelogs as to what changed

@TimKnight-DWP if it only affects an individual test feel free to also skip it and we deal with it separately. GitLab often has subtle breaking changes in the API that are not announced - hence also some of the failures here.

Have done, separated it out into a distinct test so the feature can be validated when there's chance to come bacl. I also see a fgew open MRs around which look related to some of the remaining API failures

@TimKnight-DWP also if we are religiously asserting on something that is maybe not so relevant, we could change the tests a bit to really only check what matters. Especially with deleting resources, maybe we just test something simpler if the async deletions are a pain.

Sounds like a good shout, deletions definitely seem to be one of the biggest remaining pains, and always need a bit of time to sleep.

I'm currently putting some logging in to see if there's an obvious "Scheduled for deletion" flag we can use rather than asserting thing is not there (maybe an if there -> assert scheduled for deletion, else assert has been deleted)

I think it may also. be worth validating the response from Delete APIs such as: https://docs.gitlab.com/ee/api/users.html#user-deletion

If GL returns a 204, we don't need to check that the thing has been immediately deleted, the api has told us yes, if it takes a while thats Gitlab functionality rather than python-gitlab functionaity.

If my hunch is correct now, I'll do that for these failing users tests and then take a look through and swap over

I think it may also. be worth validating the response from Delete APIs such as: https://docs.gitlab.com/ee/api/users.html#user-deletion

If GL returns a 204, we don't need to check that the thing has been immediately deleted, the api has told us yes, if it takes a while thats Gitlab functionality rather than python-gitlab functionaity.

If my hunch is correct now, I'll do that for these failing users tests and then take a look through and swap over

Ah that's a great idea @TimKnight-DWP. We shouldn't be testing GitLab behavior, just that it receives the right requests. 👍 we could probably do that with a lot of other tests too, now that I think of it.

@nejch I think with this last push that should be tests Green 🤞

I'll go through this aft and update all delete tests at least to just verify the API responded 2xx (i.e. didn't throw an exception) and then squash my commits

Should be good to go then 👍

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.49%. Comparing base (7ec3189) to head (b45f3fb).
Report is 26 commits behind head on main.

@@            Coverage Diff             @@
##             main    #2790      +/-   ##
==========================================
- Coverage   96.52%   96.49%   -0.04%     
==========================================
  Files          90       90              
  Lines        5872     5876       +4     
==========================================
+ Hits         5668     5670       +2     
- Misses        204      206       +2     

@nejch - would you be able to give this another review please? I think it'll also unblock adding integration tests for the CI Job Token PR

Showing as failing because -0.04% tests 😿

Showing as failing because -0.04% tests 😿

That's ok @TimKnight-DWP I can override that (and fix the coverage in the code that is the root cause 😅). I was focusing on something else at the moment, sorry!

@JohnVillalovos @nejch @max-wittig @bufferoverflow - this one is good to go in and then will be able to look at getting changes to modify the job_token_scope via python gitlab in, as GL have now released the APIs in a main build 👍

With regards to no longer calling "delete", gitlab have significantly changed how they handle deletes, it happens way more asynchronously, and thus is really hard to actually account for in the tests without making them even more brittle (in the case of some deletes it happens after 1 day regardless). So all we can do is assert we successfully called the delete, and we have to trust Gitlab will act on the DELETE Rest request, rather than also try and test their functionality

It's been a month, but if I recall wait_for_sidekiq did a lot of checking assuming one free worker means we're good, but with the delete changes, gitlab stores those deletes in a Q, whichcan be checked by how much concurrency sidekiq has used vs total.

But when doing so I discovered we never got close to the limits due to threading on the container, so the check became somewhat redundant. The worker always showed as "busy" because it was waiting to perform deletes, but it had spare concurrency to perform other tasks.

The sleeps could probably go away to be honest, they are mainly there to help out the stability, sometimes the container/worker are a bit slower than expected so can randomly fail a test

@JohnVillalovos @nejch - any additional thoughts on this PR?

I think it's important for us to get the tests updated to 16, but also that unlocks some later PRs to add additional, new functionality only present in 16+

Hi @nejch @JohnVillalovos @max-wittig @bufferoverflow - sorry to tag all of you and to nudge this along, but with Gitlab 17 now around the corner, I'd like to get this in so the testing suite isn't > 2 versions behind the release, and to unblock some of the JOB_TOKEN_SCOPE proposed PRs, as people may need that functionality when GitLab start to deprecate the existing Token logic in 17.x

I'm traveling at the moment, so don't have time to do the review.

Left some comments and some nits.

I'm not sure I like the changes to wait_for_sidekiq and also how it isn't called anymore in a lot of places and just replaced with sleep. I worry that this will make the tests more likely to have random failures.

Also seems like we no longer test that delete actually deletes what we want deleted, we are testing that we can call delete and no error is raised though.

But on the positive we now have tests working with Gitlab 16, which is great.

So overall I'm not sure. I would like to hear what @nejch thinks.

@JohnVillalovos I'm also not sure how I feel about removing all the deletes, but in the end it won't be very reliable with current GitLab versions if we keep them. We might lose some breaking changes in the API this way, but this would be an upstream issue.

As for wait_for_sidekiq, I also think that was maybe not very reliable. If we find a more generic way, we can also point these tests against external gitlab instances in the future.

So I'd say lets try and get this done finally 😅 we can still open follow-up issues to improve on it.

@nejch updated with your comments, but something odd seems to be happening with the github-actions, and sudden unexpected failures in the smoke tests

Thanks @TimKnight-DWP! The new smoke test failures seem unrelated as they also appear in #2833, might be something new in the build dependencies that change how the package is built, will need to check.

Thanks @TimKnight-DWP! The new smoke test failures seem unrelated as they also appear in #2833, might be something new in the build dependencies that change how the package is built, will need to check.

The culprit seems to be pypa/build#770 pypa/setuptools#3593. I'll adapt the tests.