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

Opendds4 prep - Do not merge #4779

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Conversation

jrw972
Copy link
Contributor

@jrw972 jrw972 commented Aug 22, 2024

No description provided.

@jrw972 jrw972 force-pushed the opendds4-prep branch 3 times, most recently from 93097ce to add99fe Compare August 23, 2024 19:54
@iguessthislldo
Copy link
Member

You can remove:

{
name => 'modeling',
file => "tools/modeling/tests/modeling_tests.lst",
},

@jrw972 jrw972 force-pushed the opendds4-prep branch 14 times, most recently from 1af0102 to 829b86f Compare August 30, 2024 20:04
@jrw972 jrw972 force-pushed the opendds4-prep branch 8 times, most recently from a00a6e3 to 7430505 Compare September 5, 2024 14:41
Comment on lines 377 to 380
my $self = bless {}, $class;

$self->{configs} = shift;
$self->{config} = shift;
Copy link
Member

@iguessthislldo iguessthislldo Sep 6, 2024

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:

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.


if (exists $self->{configs}->{$flag_name}) {
$self->_info("TestFramework switching to config $flag_name\n");
$self->{config} = $flag_name;
Copy link
Member

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.

my @parts = split /\//, $s;
my $key = "OPENDDS_${s}";
my $value = "\@$parts[1]";
$ENV{$key} = $value;
Copy link
Member

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.

Copy link
Contributor Author

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.

@jrw972 jrw972 force-pushed the opendds4-prep branch 3 times, most recently from 39066ec to e52b0e3 Compare September 27, 2024 14:52
@jrw972 jrw972 force-pushed the opendds4-prep branch 5 times, most recently from acc2864 to f8d7097 Compare October 11, 2024 21:07
@jrw972 jrw972 force-pushed the opendds4-prep branch 9 times, most recently from ff973f3 to af608e9 Compare October 25, 2024 16:44
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.

2 participants