Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 19, 2022
Merged

test: fix test_project_push_rules test #2332

merged 1 commit into from
Oct 19, 2022

Conversation

JohnVillalovos
Copy link
Member

Make the test_project_push_rules test work.

@JohnVillalovos JohnVillalovos requested a review from nejch October 19, 2022 05:21
Make the `test_project_push_rules` test work.
Copy link
Member

@nejch 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 raising GitlabParsingError 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

@JohnVillalovos
Copy link
Member Author

I was wondering if raising GitlabParsingError 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...

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

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

Good info!

@nejch
Copy link
Member

nejch commented Oct 19, 2022

Makes sense, I just wanted to track that somewhere which was the purpose of the xfail. I've opened an issue for that :)

@nejch nejch merged commit c676b43 into main Oct 19, 2022
@nejch nejch deleted the jlvillal/fix_test branch October 19, 2022 16:25
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