-
Notifications
You must be signed in to change notification settings - Fork 304
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
HPCC-30310 esdl ecl output to stdout #17971
Conversation
https://track.hpccsystems.com/browse/HPCC-30310 |
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.
Mostly minor comments. However, the change validate_results(self) is required as unexpected test results may be produced when stdout result is empty.
|
||
if self.expected_err != None and self.expected_err == self.result.stderr: | ||
success = True | ||
elif self.result.returncode != 0: |
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.
Should this check be done first before the expected_err != None check (in Line 153)?
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.
Not in this case. The check on 153 captures any tests that are expected to return an error by making sure the error message printed matches what is expected. Then any other test returning an error return code is a failure. I can't make an analogous check of stderr, because the esdl command prints some non-failure messages to stderr.
if self.stdout: | ||
if self.result.stdout != None and len(self.result.stdout) > 0: | ||
with open((outName / 'from-stdout.ecl'), 'w', encoding='utf-8') as f: | ||
f.write(self.result.stdout) |
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.
It should probably be do an "else return False" here after line 192. Otherwise, the tests in the next few lines will compare with previously created "from-stdout.ecl" from previous test runs.
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.
fixed
@@ -231,7 +336,15 @@ def parse_options(): | |||
help='Enable debug logging of test cases', | |||
action='store_true', default=False) | |||
|
|||
parser.add_argument('-c', '--commands', | |||
help='esdl commands to run tests for, use once for each command or pass "all" to test all commands. Defaults to "all".', | |||
action="append", choices=command_values) |
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.
Any reason not to use "default" parameter rather than the extra code in 345.
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 was my first approach, but I saw that using action="append"
with a default always results in the list containing the default in addition to any values supplied on the command line. The behavior was counter-intuitive, but the append action documentation does mention it: https://docs.python.org/3.12/library/argparse.html?highlight=argparse#action
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.
Looks good
7caa41b
to
73dddd7
Compare
@timothyklemm I've addressed the issue we talked about yesterday |
} | ||
else | ||
{ | ||
fprintf(stderr, "\noption detected before required parameters: %s\n", arg); |
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.
Why is this removed? The call to usage() produces non-ecl output but doesn't explain the specific problem.
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.
Just missed. Added back, along with another more descriptive error for missing the required parameter
Update esdl ecl command to allow output to stdout by making the outputDir parameter optional. When using stdout, restrict the use of options to those combinations that result in a single file output. Disabled verbose output in the call to EsdlCmdHelper::convertECMtoESXDL to avoid printing extra text to stdout. Update esdl regression test with cases for esdl ecl, though the suite needs independent updates to bring tests of the other commands in line with recent platform changes. This will be handled in the ticket HPCC-30699. Signed-off-by: Terrence Asselin <[email protected]>
92a36bf
to
5747ab4
Compare
@ghalliday squashed and approved to merge |
Update esdl ecl command to allow output to stdout by making the outputDir parameter optional. When using stdout, restrict the use of options to those combinations that result in a single file output.
Disabled verbose output in the call to EsdlCmdHelper::convertECMtoESXDL to avoid printing extra text to stdout.
Update esdl regression test with cases for esdl ecl, though the suite needs independent updates to bring tests of the other commands in line with recent platform changes. This will be handled in the ticket HPCC-30699.
Type of change:
Checklist:
Smoketest:
Testing:
Added regressions to the testing/esdlcmd tests. Some further work needs to be done on the prior tests in that suite, which will be handled under ticket HPCC-30699