libwww-perl / HTTP-Message

The HTTP-Message distribution contains classes useful for representing the messages passed in HTTP style communication.
https://metacpan.org/pod/HTTP::Message
Other
28 stars 61 forks source link

Extension of #164 with tests #172

Closed simbabque closed 2 years ago

simbabque commented 2 years ago

I've taken #164 and added tests. I've also included encoding for Brotli, as we support this for other compressions.

However, I am not sure how to proceed with the dependencies. I made IO::Compress::Brotli and IO::Uncompress::Brotli (both are from the same dist) suggests similar to Clone, but we require all of them for testing. The problem is that IO::Compress::Brotli requires Perl 5.14, so our github action for 5.10 and 5.12 would break.

I'm unsure how to proceed without causing breakage, so I've marked this as draft for now.

Closes #164 Fixes #163

oalders commented 2 years ago

I think we could put the Brotli tests in their own file and then use the Brotli modules via Test::Needs. That should avoid adding any extra deps.

For the dist.ini, I think we would need to put the new deps under:

[Prereqs]
-phase = test
-relationship = recommends

and

[Prereqs]
-phase = runtime
-relationship = recommends

Would be interested to hear what @haarg thinks as well.

simbabque commented 2 years ago

Thanks for the suggestion @oalders. I had to use Prereqs::Soften in the end, because it would add it as a requires with version 0 as well automatically.

Looks like it's now failing the ones with release testing set, because Test::Needs fails if that's set and the module is missing. I had expected it to just install. I'll investigate.

simbabque commented 2 years ago

Turns out --with-recommends was missing from the cpm command in the Linux build job. But when I add that, we get the predicted fail on 5.10.

oalders commented 2 years ago

I think we can skip release testing outside of the initial dzil build. I expect anyone who wants to release to be on a recent Perl. 😀

simbabque commented 2 years ago

That makes sense. I've done that. But it doesn't fix the issue that cpm doesn't know that it can't install the new dependency on machines that are lower than Perl 5.14.

I could make the --with-recommends dependant on the perl-version, but that doesn't feel very future-proof.

oalders commented 2 years ago

I could make the --with-recommends dependant on the perl-version, but that doesn't feel very future-proof.

How so? If the workflow is just checking for a version > x that should be fine?

simbabque commented 2 years ago

Ah, there's a syntax error in the action now. I got it to work for Linux, but mac isn't working yet. I'll fix that tomorrow.

simbabque commented 2 years ago

This is all done now, we can merge.