-
Notifications
You must be signed in to change notification settings - Fork 482
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
Opendds4 prep - Do not merge #4779
base: master
Are you sure you want to change the base?
Conversation
93097ce
to
add99fe
Compare
You can remove: OpenDDS/tests/auto_run_tests.pl Lines 89 to 92 in 00d1d5c
|
1af0102
to
829b86f
Compare
a00a6e3
to
7430505
Compare
7430505
to
81a5bf7
Compare
bin/PerlDDS/Run_Test.pm
Outdated
my $self = bless {}, $class; | ||
|
||
$self->{configs} = shift; | ||
$self->{config} = shift; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if Perl has a term for this, but if these are optional consider making these "keyword" arguments as Python calls them. This way they are explicit and it makes it much easier to add more arguments in the future in a backwards compatible way. The idea is because arrays and hashes are basically the same thing in Perl you can do:
my $test = new PerlDDS::TestFramework(configs => \%configs, config => 'ir_tcp');
Then @_
can be treated as a hash that can be joined with a default argument hash.
Here's an example of this I've implemented before:
OpenDDS/tools/scripts/modules/command_utils.pm
Lines 264 to 282 in 07ee200
my %valid_args = ( | |
name => undef, | |
capture => {}, | |
verbose => 0, | |
verbose_fh => *STDERR, | |
dry_run => 0, | |
script_name => undef, | |
error_fh => *STDERR, | |
exit_status => undef, | |
ignore => [], | |
ignore_all => 0, | |
ret_exit_status => 0, | |
chdir => undef, | |
autodie => 0, | |
); | |
my %args = (%valid_args, @_); | |
my @invalid_args = grep { !exists($valid_args{$_}) } keys(%args); | |
trace("invalid arguments: ", join(', ', @invalid_args)) if (scalar(@invalid_args)); |
Here I'd recommend something like:
# At the top we'd need to import trace since we're currently
# not using in the test framework, but really should:
use FindBin;
use lib "$FindBin::RealBin/../../tools/scripts/modules"
use misc_utils qw(trace);
# ...
# At where I'm commenting replace the bless, configs, and config lines with:
my %valid_args = (
configs => {},
config => undef,
);
my %args = (%valid_args, @_);
my @invalid_args = grep { !exists($valid_args{$_}) } keys(%args);
trace("invalid arguments: ", join(', ', @invalid_args)) if (scalar(@invalid_args));
my $self = bless(\%args, $class);
Because args
is the basis of the bless
, configs
and config
will be in $self
. We could also move most or all of the other values below into the valid_args
hash to allow overriding them from the constructor.
bin/PerlDDS/Run_Test.pm
Outdated
|
||
if (exists $self->{configs}->{$flag_name}) { | ||
$self->_info("TestFramework switching to config $flag_name\n"); | ||
$self->{config} = $flag_name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this and many Perl files aren't consistent, but we should prefer 2 spaces to 4 spaces.
bin/PerlDDS/Run_Test.pm
Outdated
my @parts = split /\//, $s; | ||
my $key = "OPENDDS_${s}"; | ||
my $value = "\@$parts[1]"; | ||
$ENV{$key} = $value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this isn't going to work in a remote-controlled cross-compiled target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. PerlACE TestTargets have SetEnv
which we can use to set environment variables in this case.
39066ec
to
e52b0e3
Compare
acc2864
to
f8d7097
Compare
ff973f3
to
af608e9
Compare
No description provided.