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

cass: support newer HTTP::Tiny, Net::DAVTalk, Mail::JMAPTalk #5229

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

elliefm
Copy link
Contributor

@elliefm elliefm commented Jan 22, 2025

Newer HTTP::Tiny turns on SSL certificate verification by default, which meant tests using HTTPS were failing. This was previously worked around in #5227.

There have since been updates to Mail::JMAPTalk and Net::DAVTalk to pass through our SSL_options, so if the new versions of those are installed we can skip the workaround and do it properly instead, with certificate verification enabled.

Also added a simple test that exercises JMAP-over-HTTPS, because this was broken just like DAV-over-HTTPS was, just not visibly so because nothing used it.

@elliefm elliefm force-pushed the v313/cyr-1526-net-davtalk-update branch 3 times, most recently from 8e49577 to 6cc26ca Compare January 29, 2025 00:21
@elliefm elliefm changed the title WIP TestCase: pass SSL_options to Net::DAVTalk cass: support newer HTTP::Tiny, Net::DAVTalk, Mail::JMAPTalk Jan 29, 2025
@elliefm elliefm added the backport-to-3.12 for PRs that are to be backported to 3.12 label Jan 29, 2025
@elliefm elliefm marked this pull request as ready for review January 29, 2025 00:40
Copy link
Member

@rsto rsto left a comment

Choose a reason for hiding this comment

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

Works for me:

$ perl -MHTTP::Tiny -le 'print $HTTP::Tiny::VERSION'
0.080

$ perl -MNet::DAVTalk -le 'print $Net::DAVTalk::VERSION'
0.2

$ perl -MMail::JMAPTalk -le 'print $Mail::JMAPTalk::VERSION'
0.15

./testrunner.pl --config ./cassandane.ini -j 4 -f pretty JMAPCore.echo_tls
[ OK ] Cyrus::JMAPCore.echo_tls

#!perl
use Cassandane::Tiny;

sub test_echo_tls
Copy link
Member

Choose a reason for hiding this comment

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

The file name contains a dash but the test name an underscore. Please change the file name to match the test name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rik usually prefers file names not have underscores because they can be hard to see in terminal output or other situations, so we use dashes instead.

Perl subroutines can't contain dashes, so we use underscores instead.

I think it would be fair to say files use dashes and subs use underscores. We have a mix of both right now but settling on one would be better.

What do you think @rsto?

Copy link
Contributor

@ksmurchison ksmurchison left a comment

Choose a reason for hiding this comment

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

WFM

$ perl -MHTTP::Tiny -le 'print $HTTP::Tiny::VERSION'
0.090
$ perl -MNet::DAVTalk -le 'print $Net::DAVTalk::VERSION'
0.22
$ perl -MMail::JMAPTalk -le 'print $Mail::JMAPTalk::VERSION'
0.15

$ ./testrunner.pl -j9 ALPN JMAPCore.echo_tls
[ OK ] Cyrus::ALPN.imap_bad
[ OK ] Cyrus::ALPN.imap_good
[ OK ] Cyrus::ALPN.https_bad
[ OK ] Cyrus::ALPN.https_h2
[ OK ] Cyrus::ALPN.https_multi2
[ OK ] Cyrus::ALPN.https_multi
[ OK ] Cyrus::ALPN.https_http10
[ OK ] Cyrus::ALPN.https_http11
[ OK ] Cyrus::ALPN.imaps_bad
[ OK ] Cyrus::ALPN.https_none
[ OK ] Cyrus::ALPN.imap_none
[ OK ] Cyrus::ALPN.imaps_good
[ OK ] Cyrus::ALPN.imaps_none
[ OK ] Cyrus::JMAPCore.echo_tls

Time: 6 wallclock secs ( 0.28 usr 0.07 sys + 3.50 cusr 3.02 csys = 6.87 CPU)

OK (14 tests)

Copy link
Contributor

@wolfsage wolfsage left a comment

Choose a reason for hiding this comment

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

If this works, seems fine to me.

An alternate option to consider would be to require the latest of everything. Folks will have to update some modules once, and then we don't have this confusing situation anymore?

#!perl
use Cassandane::Tiny;

sub test_echo_tls
Copy link
Contributor

Choose a reason for hiding this comment

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

Rik usually prefers file names not have underscores because they can be hard to see in terminal output or other situations, so we use dashes instead.

Perl subroutines can't contain dashes, so we use underscores instead.

I think it would be fair to say files use dashes and subs use underscores. We have a mix of both right now but settling on one would be better.

What do you think @rsto?

@rsto
Copy link
Member

rsto commented Jan 30, 2025

@wolfsage In the case of Cassandane/tiny-tests I prefer we keep using underscore also in the file name. I'm surprised we do have file names with dashes because Rik even renamed the test files from using dash to underscore after we agreed to use underscores: 115323e

The reason for that is that it's more ergonomic being able to copy the name of a failing test and paste that name into the command line, without having to replace all underscores.

@wolfsage
Copy link
Contributor

The reason for that is that it's more ergonomic being able to copy the name of a failing test and paste that name into the command line, without having to replace all underscores.

Okay, works for me!

@elliefm
Copy link
Contributor Author

elliefm commented Jan 30, 2025

If this works, seems fine to me.

An alternate option to consider would be to require the latest of everything. Folks will have to update some modules once, and then we don't have this confusing situation anymore?

Yep, I agree, except that we don't use Foo; these modules in this file, so we can't use Foo <version>; them. We'd need to do require Foo; Foo->VERSION('<version>'); or something, which I tried briefly and hated.

The reason we don't just use Foo; in this file is so that when Cyrus was built without http support, someone running Cassandane isn't forced to install a bunch of perl modules that won't actually be used (because all the tests that would care will be skipped anyway).

We intend at some point to make http support non-optional; at that point, this mess can go away and we can just use Foo <version>; these modules at the top like usual.

@elliefm elliefm force-pushed the v313/cyr-1526-net-davtalk-update branch from 6cc26ca to 0d60561 Compare January 30, 2025 22:45
@elliefm elliefm requested a review from rsto January 30, 2025 22:45
@elliefm
Copy link
Contributor Author

elliefm commented Jan 30, 2025

@rsto I've renamed the echo-tls file to echo_tls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-3.12 for PRs that are to be backported to 3.12
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants