NotificationsYou must be signed in to change notification settings
Fork673
Star2.4k

test: fix`test_project_push_rules` test#2332

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nejch merged 1 commit intomainfromjlvillal/fix_test

Oct 19, 2022

Merged

test: fix`test_project_push_rules` test#2332

nejch merged 1 commit intomainfromjlvillal/fix_test

Oct 19, 2022

Conversation

Copy link

Member

JohnVillalovos commentedOct 19, 2022

Make thetest_project_push_rules test work.

JohnVillalovos requested a review fromnejch

October 19, 2022 05:21

test: fixtest_project_push_rules test

8779cf6

Make the `test_project_push_rules` test work.

JohnVillalovos force-pushed thejlvillal/fix_test branch fromfbc8436 to8779cf6Compare

October 19, 2022 05:22

nejch reviewed

Oct 19, 2022

View reviewed changes

Copy link

Member

nejch left a comment

Choose a reason for hiding this comment

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

I was wondering if raisingGitlabParsingError is correct for a "valid" response, IIRC we added it for malformed responses and not for real GitLab responses (although here I would expect an empty dict or 404 TBH). Maybe a more specific error or an upstream fix to not return None would be clearer to users.

Here is an old issue that I forgot about completely:#1192

And the upstream issue:https://gitlab.com/gitlab-org/gitlab/-/issues/8829

Copy link

MemberAuthor

JohnVillalovos commentedOct 19, 2022

I was wondering if raisingGitlabParsingError is correct for a "valid" response, IIRC we added it for malformed responses and not for real GitLab responses (although here I would expect an empty dict or 404 TBH). Maybe a more specific error or an upstream fix to not return None would be clearer to users.

I doubt it is the correct but it is what we are currently doing. My goal was to fix the test at this stage, not to fix the underlying issue which I think will be harder to fix...