Your code is not actually very complex. You don't actually access anything very specific. It's just a hash of arguments. The correct thing to do is to test for these cases:
- called without args
- called with wrong args
- called with a value that doesn't match the pattern (will fail, you don't account for that)
- called with URL with trailing slash with port
- called with URL without trailing slash with port
- called with URL with trailing slash without port
- called with URL without trailing slash without port
Those tests are all fairly simple.
use Test::More;
use strict;
use warnings;
my $foo = Foo->new;
is( $foo->_get_api_host_from_environment(),
undef, 'return undef without arguments' );
is( $foo->_get_api_host_from_environment( foo => 123 ),
undef, 'return undef with missing environment' );
{
local $TODO = 'this check is not implemented yet';
is(
$foo->_get_api_host_from_environment(
environment => 'this is not a URL'
),
undef,
'environment is not a URL'
);
}
is(
$foo->_get_api_host_from_environment(
environment => 'http://example.org:8174/'
),
'example.org:28174',
'port number and trailing slash prefixes 2 to the port'
);
is(
$foo->_get_api_host_from_environment(
environment => 'http://example.org:8174'
),
'example.org:28174',
'port number w/o trailing slash prefixes 2 to the port'
);
is(
$foo->_get_api_host_from_environment(
environment => 'http://example.org/'
),
'example.org:4040',
'no port number and trailing slash adds port 4040'
);
is(
$foo->_get_api_host_from_environment(
environment => 'http://example.org'
),
'example.org:4040',
'no port number w/o trailing slash adds port 4040'
);
done_testing;
Note how my tests all have descriptive names. Good tests double as documentation for the developer that will maintain the code in the future (could be you in a year), so make sure you actually talk about what and why you are testing in your descriptions.
There's a $TODO in there to show that the check for an incorrect URL has not been built yet. That test will fail, but the test suite will pass anyway because it's expected to fail.
In order to make these tests pass, I needed to add some code around your sub. This is what I did.
package Foo;
use strict;
use warnings;
sub new { return bless {}, $_[0] }
sub missing_params {
my $self = shift;
my $args = shift;
foreach my $param (@_) {
return 1 unless exists $args->{$param};
}
return;
}
sub _get_api_host_from_environment {
my ( $self, %args ) = @_;
$self->missing_params( \%args, qw! environment! )
and return;
$args{environment} =~ m{(?:https?://)?([^:\/]+):?([^\/]*)}
; # http://m5devacoe01.gcsc.att.com:8174/
my $host = $1; # m5devacoe01.gcsc.att.com
my $port = $2; # 8174
# If port is provided it means that it's a development server request. So, append 2 in the port
if ($port) {
$host .= ':2' . $port;
}
else {
$host .= ':4040';
}
return $host;
}
You will notice how I changed your regular expression. That's because yours was actually wrong and didn't do what your documentation says the code does. The new pattern introduces the option of having https instead of http as the scheme, which I did not have tests for, as well as not having the scheme at all. I did not include tests for these cases, but they should really be there as well.
It might be smarter to actually use the URI module for this. It allows you to modify the port directly, and gives you free sanity checks.
sub _get_api_host_from_environment {
my ( $self, %args ) = @_;
$self->missing_params( \%args, qw! environment! )
and return;
my $uri = URI->new( $args{environment} ) or return;
return unless $uri->scheme && $uri->scheme =~ 'http'; # includes https
my $port = $uri->port; # 8174
# If port is provided it means that it's a development server request. So, append 2 in the port
if ($port != 80 and $port != 443) {
$uri->port( '2' . $port );
}
else {
$uri->port('4040');
}
# return $uri->canonical; # this would return http://example.org:28174/
return $uri->host_port;
}
With this test, the same unit tests will still pass.
