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

scripts/test.sh: Fix auto_into_responses feature declaration #1252

Merged
merged 3 commits into from
Dec 21, 2024

Conversation

Turbo87
Copy link
Contributor

@Turbo87 Turbo87 commented Dec 20, 2024

#665 renamed the feature, but for some reason the test.sh script contained utoipa/ prefixes for the feature name in some cases, which effectively disabled these tests from even getting compiled.

This PR fixes the feature name in the cargo test invocations and "fixes" the actix tests as best as I was able to. Two tests weren't compiling at all, and another couple of tests were resulting in different output than the test was expecting. This should probably be fixed properly by someone with more knowledge of actix than me, but since it's already broken on master I guess we may as well merge it in this slightly less broken state 😅

These invocations weren't actually running the corresponding tests
@juhaku
Copy link
Owner

juhaku commented Dec 21, 2024

#665 renamed the feature, but for some reason the test.sh script contained utoipa/ prefixes for the feature name in some cases, which effectively disabled these tests from even getting compiled.

This PR fixes the feature name in the cargo test invocations and "fixes" the actix tests as best as I was able to. Two tests weren't compiling at all, and another couple of tests were resulting in different output than the test was expecting. This should probably be fixed properly by someone with more knowledge of actix than me, but since it's already broken on master I guess we may as well merge it in this slightly less broken state 😅

Yup, It's quite long ago, since that feature was on the table. 😅 Thus no wonder if there are some tests that does not work as expected. So I agree we can merge this as is in slightly broken state and whenever the automatic responses will be looked again, it is more likely beneficial start fresh while keeping the original attempt as a reference. Anyhow that is something for some unknown future 😄 I believe that probably no one has ever used that feature.

@@ -13,6 +13,7 @@

### Changed

* scripts/test.sh: Fix `auto_into_responses` feature declaration (https://github.com/juhaku/utoipa/pull/1252)
Copy link
Owner

Choose a reason for hiding this comment

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

Otherwise great, but should be under new title Unreleased and subtitle Changed at the top before 5.3.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, I missed that this was for a released version 😅

Copy link
Owner

@juhaku juhaku left a comment

Choose a reason for hiding this comment

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

Looks fine and dandy, the auto_into_responses is something that needs to be looked at in the future.

@juhaku juhaku merged commit 6d8d2ac into juhaku:master Dec 21, 2024
12 checks passed
@Turbo87 Turbo87 deleted the broken-tests branch December 22, 2024 12:08
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