Skip to content

Use the inputs when testing URI conversions in Http4sConversionSpec#1666

Merged
kubukoz merged 1 commit intodisneystreaming:series/0.18from
Dwolla:Http4sConversionSpec
Mar 31, 2025
Merged

Use the inputs when testing URI conversions in Http4sConversionSpec#1666
kubukoz merged 1 commit intodisneystreaming:series/0.18from
Dwolla:Http4sConversionSpec

Conversation

@bpholt
Copy link
Copy Markdown
Contributor

@bpholt bpholt commented Mar 25, 2025

Not a huge deal, but I discovered that Http4sConversionSpec#http4sToSmithyAndBackUriTest wasn't using its input in its test setup, so most of the tests were redundant. I believe this was the intended test behavior.

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

1 similar comment
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.


http4sToSmithyAndBackUriTest(
uri"example.com",
uri"//example.com",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Without the leading //, http4s parses this as the URI path, so the test fails because it was essentially trying to compare http://localhost/example.com to http://example.com.

Copy link
Copy Markdown
Member

@kubukoz kubukoz Mar 31, 2025

Choose a reason for hiding this comment

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

i've had the "pleasure" of hitting this... yeah it's fun

@kubukoz
Copy link
Copy Markdown
Member

kubukoz commented Mar 31, 2025

pretty sure the CLA isn't needed for a change like this

@kubukoz kubukoz merged commit 3297511 into disneystreaming:series/0.18 Mar 31, 2025
10 of 11 checks passed
@kubukoz kubukoz mentioned this pull request Mar 31, 2025
7 tasks
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.

3 participants