-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
ROSCI basic infra to enable single live_cam test #3076
Conversation
@@ -0,0 +1,144 @@ | |||
# Copyright 2023 Intel Corporation. All Rights Reserved. |
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.
2024
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.
Corrected in commit # 3
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.
GHA failing for 2024, hence reverted to 2023 again w/ commit # 4
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.
@Arun-Prasad-V / @SamerKhshiboun haven't we fixed that to be generic?
run_time = time.time() - start_time | ||
log.d( "server took", run_time, "seconds" ) | ||
|
||
sys.exit( 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.
Will the current script return ~= 0 if the test failed and 0 if it pass?
Can you add a screen shot of the running output?
Thanks
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.
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.
Yes, that's right. Made it little more clear in commit # 3.
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.
Test ROS stage fails when the subprocess.run() returns non zero exit status. However, there is NO log processing mechanism implemented yet as to why the test failed. For now, logs have to be checked manually if test fails @ "Build artifacts": ros2/realsense2_camera/log/
start_time = time.time() | ||
current_dir = os.path.dirname( os.path.abspath( __file__ ) ) | ||
print(f'{current_dir}') | ||
root = os.path.dirname( os.path.dirname( os.path.dirname( os.path.dirname( os.path.dirname( os.path.abspath( __file__ )))))) |
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.
This looks very weird, what's going on here?
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.
restructured in commit # 5
cmd += ['-s'] | ||
cmd += ['-m', ''.join(dev_name)] | ||
cmd += ['-k', 'test_camera_imu_tests'] | ||
#cmd += ['-m', 'd415'] |
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.
Redundant?
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.
cleaned up in commit # 5
universal_newlines=True, | ||
timeout=200, | ||
check=True ) | ||
if not result.returncode: |
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.
What if we get an error code?
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 error code !=0 invokes exception and thereby CI fails
device_set = None | ||
try: | ||
opts, args = getopt.getopt( sys.argv[1:], 'hvqr:st:', | ||
longopts=['help', 'verbose', 'debug', 'quiet', 'regex=', 'stdout', 'tag=', 'list-tags', |
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.
Maybe only add option you support?
And on next PR's add options and implemantation?
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.
cleaned up in commit # 5. Not supporting any option for this PR. Test name and device name hard-coded for this PR.
device_set = arg.split() | ||
for dev in device_set: | ||
print(dev) | ||
#Get into 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.
?
else: | ||
devices.hub.connect() | ||
devices.query( hub_reset = hub_reset ) | ||
#devices.map_unknown_ports() |
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.
?
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.
cleaned-up
assert False, 'No Camera device detected!' | ||
else: | ||
#Loop in for all devices and run tests | ||
for device in devices._device_by_sn.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.
Why not add a devices.py like in LibCI and query devices?
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.
Yes, query devices thru devices.py module which stores the device data in dict -- '_device_by_sn' , and then processing of data happens here
print('Running test on device:', device.name) | ||
cmd = command(str(device.name).lower()) | ||
run_test(cmd, device.name, stdout=logdir, append =False) | ||
elif device.name != 'D455': |
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.
This elif looks redundant, if it's not D455 it's not D455 :)
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.
cleaned-up
cmd = ['pytest-3'] | ||
cmd += ['-s'] | ||
cmd += ['-m', ''.join(dev_name)] | ||
cmd += ['-k', 'test_camera_imu_tests'] |
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 understand for now, the test name is hardcoded. But shall we hardcode the things outside and have this function generic? So that you don't need to touch this function later.
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.
Next PR will carry these these fixes
def run_test(cmd, dev_name, stdout=None, append =False): | ||
handle = None | ||
try: | ||
stdout = stdout + os.sep + str(dev_name) + '_' + "test_camera_imu_tests" + ".log" |
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.
same here...
#Loop in for all devices and run tests | ||
for device in devices._device_by_sn.values(): | ||
log.i(f'Device found: {device.name} ') | ||
if device.name == 'D455': |
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.
same here...
How about Arun's comments? |
|
Going to create a fresh PR with additional features |
Initial version of ROSCI script with limited functionality enabling single live camera test. Target device (D455), Test name, and paths hardcoded for now. This version is intended to deploy ROSCI w/ one live cam test, and no features added yet.