From d343a63c17f89f1a2b88ee374bb2b27d206fe29d Mon Sep 17 00:00:00 2001 From: JulienThevenoz Date: Tue, 5 Dec 2023 16:42:21 +0100 Subject: [PATCH 1/7] clean processes automatically at exit; replaced custom Waiter class with Popen.wait() function --- systemtests/run_test.py | 50 ++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/systemtests/run_test.py b/systemtests/run_test.py index 73cfa1406..ce1f5c905 100755 --- a/systemtests/run_test.py +++ b/systemtests/run_test.py @@ -1,6 +1,6 @@ #!/usr/bin/env python3 #!/usr/bin/env python3 -from subprocess import Popen, PIPE +from subprocess import Popen, PIPE, TimeoutExpired import time import os import signal @@ -12,25 +12,18 @@ import atexit -class Waiter: - '''A helper class which sleeps the amount of seconds it is instanciated with. If the user interrupts with ^C during the waiting time (zB if the drone crashes), it will stop sleeping and continue - to the end of the program where all child processes are terminated correctly. All subsequent Waiter() calls will be ignored''' - wait=True - - def __init__(self, seconds): - if seconds > 0 : - self.sleep(seconds) - - def sleep(self, seconds): - start=time.time() - while Waiter.wait and time.time()<(start+seconds): - try: - time.sleep(1) - except KeyboardInterrupt: - Waiter.wait = False +def clean_process(process:Popen): + '''Kills process and its children on exit if they aren't already terminated (called with atexit)''' + group_id = os.getpgid(process.pid) + if process.poll() == None: + print(f"cleaning {group_id}") + os.killpg(group_id, signal.SIGTERM) + time.sleep(0.01) + if process.poll() == None: + print(f"Process group {group_id} didn't terminate correctly") def record_start_and_terminate(testname:str, testduration:int, bagfolder:str): - '''Helper function that starts recording the /tf topic in a rosbag, starts the test, waits, closes the rosbag and terminate all processes + '''Starts recording the /tf topic in a rosbag, starts the test, waits, closes the rosbag and terminate all processes NB the testname must be the name of the crayzflie_examples executable (ie the CLI grammar "ros2 run crazyflie_examples testname" must be valid)''' index = bagfolder.find("bag_") bagname = bagfolder[index:] @@ -41,16 +34,23 @@ def record_start_and_terminate(testname:str, testduration:int, bagfolder:str): command = f"{src} && ros2 bag record -s mcap -o {testname}_{bagname} /tf" record_bag = Popen(command, shell=True, cwd=bagfolder, stderr=PIPE, stdout=True, start_new_session=True, text=True, executable="/bin/bash") + atexit.register(clean_process, record_bag) command = f"{src} && ros2 run crazyflie_examples {testname}" start_flight_test = Popen(command, shell=True, stderr=True, stdout=True, start_new_session=True, text=True, executable="/bin/bash") + atexit.register(clean_process, start_flight_test) + try: + start_flight_test.wait(timeout=testduration) #wait x seconds for the crazyflie to fly the test - Waiter(testduration) #wait x seconds for the crazyflie to fly the test + except TimeoutExpired: #when finished waiting + os.killpg(os.getpgid(start_flight_test.pid), signal.SIGTERM) #kill flight test and all of its child processes + os.killpg(os.getpgid(record_bag.pid), signal.SIGTERM) #kill rosbag - os.killpg(os.getpgid(start_flight_test.pid), signal.SIGTERM) #kill flight test and all of its child processes - os.killpg(os.getpgid(record_bag.pid), signal.SIGTERM) #kill rosbag + except KeyboardInterrupt: #if drone crashes, user can ^C to skip the waiting + os.killpg(os.getpgid(start_flight_test.pid), signal.SIGTERM) #kill flight test and all of its child processes + os.killpg(os.getpgid(record_bag.pid), signal.SIGTERM) #kill rosbag #if something went wrong with the bash command lines in Popen, print the error if record_bag.stderr != None: @@ -61,7 +61,7 @@ def record_start_and_terminate(testname:str, testduration:int, bagfolder:str): def translate_and_plot(testname:str, bagfolder:str): - '''Helper function that translates rosbag .mcap format to .csv, then uses that csv to plot a pdf ''' + '''Translates rosbag .mcap format to .csv, then uses that csv to plot a pdf ''' index = bagfolder.find("bag_") bagname = bagfolder[index:] # NB : the mcap filename is almost the same as the folder name but has _0 at the end @@ -83,7 +83,7 @@ def translate_and_plot(testname:str, bagfolder:str): if __name__ == "__main__": - path = Path(__file__) #Path(__file__) should be "/home/github/actions-runner/_work/crazyswarm2/crazyswarm2/ros2_ws/src/crazyswarm2/systemtests/newsub.py" ; path.parents[0]=.../systemstests + path = Path(__file__) #Path(__file__) in this case "/home/github/actions-runner/_work/crazyswarm2/crazyswarm2/ros2_ws/src/crazyswarm2/systemtests/newsub.py" ; path.parents[0]=.../systemstests #delete results, logs and bags of previous experiments shutil.rmtree(path.parents[3].joinpath("bagfiles")) @@ -102,13 +102,13 @@ def translate_and_plot(testname:str, bagfolder:str): command = f"{src} && ros2 launch crazyflie launch.py" launch_crazyswarm = Popen(command, shell=True, stderr=True, stdout=True, text=True, start_new_session=True, executable="/bin/bash") + atexit.register(clean_process, launch_crazyswarm) time.sleep(1) - print("f8") record_start_and_terminate("figure8", 20, bagfolder) - print("multi") record_start_and_terminate("multi_trajectory", 80, bagfolder) + os.killpg(os.getpgid(launch_crazyswarm.pid), signal.SIGTERM) #kill crazyswarm and all of its child processes From 99a2580e8668e400df5cf87d2cf18287b183fd04 Mon Sep 17 00:00:00 2001 From: JulienThevenoz Date: Tue, 5 Dec 2023 16:52:15 +0100 Subject: [PATCH 2/7] reduced time delay in clean_process --- systemtests/run_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/systemtests/run_test.py b/systemtests/run_test.py index ce1f5c905..7430ec4d6 100755 --- a/systemtests/run_test.py +++ b/systemtests/run_test.py @@ -18,7 +18,7 @@ def clean_process(process:Popen): if process.poll() == None: print(f"cleaning {group_id}") os.killpg(group_id, signal.SIGTERM) - time.sleep(0.01) + time.sleep(0.001) #necessary delay for next step to work correctly if process.poll() == None: print(f"Process group {group_id} didn't terminate correctly") From 8d3b7baadc3c016c4f829493dd9c89d3d0f712d4 Mon Sep 17 00:00:00 2001 From: JulienThevenoz Date: Tue, 5 Dec 2023 16:57:10 +0100 Subject: [PATCH 3/7] added the branch to trigger the github action --- .github/workflows/systemtests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/systemtests.yml b/.github/workflows/systemtests.yml index 21588070b..f4588e9ff 100644 --- a/.github/workflows/systemtests.yml +++ b/.github/workflows/systemtests.yml @@ -2,7 +2,7 @@ name: System Tests on: push: - branches: [ "feature_systemtests" ] + branches: [ "feature_systemtests_better" ] # manual trigger workflow_dispatch: From 12ee25d79552418352aa2c911787a39128a8bd8f Mon Sep 17 00:00:00 2001 From: JulienThevenoz Date: Tue, 5 Dec 2023 16:59:08 +0100 Subject: [PATCH 4/7] added the branch to trigger the github action : typo --- .github/workflows/systemtests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/systemtests.yml b/.github/workflows/systemtests.yml index f4588e9ff..da29294a4 100644 --- a/.github/workflows/systemtests.yml +++ b/.github/workflows/systemtests.yml @@ -2,7 +2,7 @@ name: System Tests on: push: - branches: [ "feature_systemtests_better" ] + branches: [ "feature-systemtests-better" ] # manual trigger workflow_dispatch: From e6dcf27b4ba7083713223cf63b67160420d205cd Mon Sep 17 00:00:00 2001 From: JulienThevenoz Date: Thu, 7 Dec 2023 10:32:54 +0100 Subject: [PATCH 5/7] test scripts should finish correctly, clean_process() has a returncode --- systemtests/run_test.py | 73 ++++++++++++++++++++++++++--------------- 1 file changed, 46 insertions(+), 27 deletions(-) diff --git a/systemtests/run_test.py b/systemtests/run_test.py index 7430ec4d6..4d48597fb 100755 --- a/systemtests/run_test.py +++ b/systemtests/run_test.py @@ -12,45 +12,64 @@ import atexit -def clean_process(process:Popen): - '''Kills process and its children on exit if they aren't already terminated (called with atexit)''' - group_id = os.getpgid(process.pid) +def clean_process(process:Popen) -> int : + '''Kills process and its children on exit if they aren't already terminated (called with atexit). Returns 0 on success, 1 on failure''' if process.poll() == None: - print(f"cleaning {group_id}") + group_id = os.getpgid(process.pid) + print(f"cleaning process {group_id}") os.killpg(group_id, signal.SIGTERM) - time.sleep(0.001) #necessary delay for next step to work correctly - if process.poll() == None: - print(f"Process group {group_id} didn't terminate correctly") + time.sleep(0.01) #necessary delay before first poll + i=0 + while i < 10 and process.poll() == None: #in case the process termination is lazy and takes some time, we wait up to 0.5 sec per process + if process.poll() != None: + print("yaY") + return 0 #if we have a returncode-> it terminated + time.sleep(0.05) #if not wait a bit longer + if(i == 9): + print(f"Process group {process} with groupID {group_id} didn't terminate correctly") + return 1 #after 0.5s we stop waiting and consider it did not terminate correctly + return 0 + else: + print(process.returncode) + return 0 #process already terminated -def record_start_and_terminate(testname:str, testduration:int, bagfolder:str): - '''Starts recording the /tf topic in a rosbag, starts the test, waits, closes the rosbag and terminate all processes +def record_start_and_terminate(testname:str, max_wait:int, bagfolder:str): + '''Starts recording the /tf topic in a rosbag, starts the test, waits, closes the rosbag and terminate all processes. max_wait is the max amount of time you want to wait + before forcefully terminating the test flight script (in case it never finishes correctly). NB the testname must be the name of the crayzflie_examples executable (ie the CLI grammar "ros2 run crazyflie_examples testname" must be valid)''' index = bagfolder.find("bag_") bagname = bagfolder[index:] src = "source " + bagfolder[:index-9] + "install/setup.bash" # -> "source /home/github/actions-runner/_work/crazyswarm2/crazyswarm2/ros2_ws/install/setup.bash" - - command = f"{src} && ros2 bag record -s mcap -o {testname}_{bagname} /tf" - record_bag = Popen(command, shell=True, cwd=bagfolder, stderr=PIPE, stdout=True, - start_new_session=True, text=True, executable="/bin/bash") - atexit.register(clean_process, record_bag) - - command = f"{src} && ros2 run crazyflie_examples {testname}" - start_flight_test = Popen(command, shell=True, stderr=True, stdout=True, - start_new_session=True, text=True, executable="/bin/bash") - atexit.register(clean_process, start_flight_test) - try: - start_flight_test.wait(timeout=testduration) #wait x seconds for the crazyflie to fly the test + command = f"{src} && ros2 bag record -s mcap -o {testname}_{bagname} /tf" + record_bag = Popen(command, shell=True, cwd=bagfolder, stderr=PIPE, stdout=True, + start_new_session=True, text=True, executable="/bin/bash") + atexit.register(clean_process, record_bag) + + command = f"{src} && ros2 run crazyflie_examples {testname}" + start_flight_test = Popen(command, shell=True, stderr=True, stdout=True, + start_new_session=True, text=True, executable="/bin/bash") + atexit.register(clean_process, start_flight_test) + + + # if start_flight_test.returncode == 0: #if the flight test script finishes before max_wait, we still want to + # raise TimeoutExpired(command, max_wait) + start_flight_test.wait(timeout=max_wait) #raise Timeoutexpired after max_wait seconds if start_flight_test didn't finish by itself + clean_process(start_flight_test) + clean_process(record_bag) + print("normal finish") - except TimeoutExpired: #when finished waiting - os.killpg(os.getpgid(start_flight_test.pid), signal.SIGTERM) #kill flight test and all of its child processes - os.killpg(os.getpgid(record_bag.pid), signal.SIGTERM) #kill rosbag + except TimeoutExpired: #if max_wait is exceeded + clean_process(start_flight_test) + clean_process(record_bag) + print("timeout finish") except KeyboardInterrupt: #if drone crashes, user can ^C to skip the waiting - os.killpg(os.getpgid(start_flight_test.pid), signal.SIGTERM) #kill flight test and all of its child processes - os.killpg(os.getpgid(record_bag.pid), signal.SIGTERM) #kill rosbag + clean_process(start_flight_test) + clean_process(record_bag) + print("keyboard interrupt finish") #if something went wrong with the bash command lines in Popen, print the error if record_bag.stderr != None: @@ -109,7 +128,7 @@ def translate_and_plot(testname:str, bagfolder:str): record_start_and_terminate("multi_trajectory", 80, bagfolder) - os.killpg(os.getpgid(launch_crazyswarm.pid), signal.SIGTERM) #kill crazyswarm and all of its child processes + clean_process(launch_crazyswarm) #kill crazyswarm and all of its child processes #test done, now we create the results pdf From 52d35fb132ec91ac8dbeacf59122ab21044ea28f Mon Sep 17 00:00:00 2001 From: JulienThevenoz Date: Thu, 7 Dec 2023 11:00:56 +0100 Subject: [PATCH 6/7] delete results/bags/logs folders only if they exist --- systemtests/run_test.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/systemtests/run_test.py b/systemtests/run_test.py index 4d48597fb..3271e4d90 100755 --- a/systemtests/run_test.py +++ b/systemtests/run_test.py @@ -104,10 +104,13 @@ def translate_and_plot(testname:str, bagfolder:str): path = Path(__file__) #Path(__file__) in this case "/home/github/actions-runner/_work/crazyswarm2/crazyswarm2/ros2_ws/src/crazyswarm2/systemtests/newsub.py" ; path.parents[0]=.../systemstests - #delete results, logs and bags of previous experiments - shutil.rmtree(path.parents[3].joinpath("bagfiles")) - shutil.rmtree(path.parents[3].joinpath("results")) - shutil.rmtree(Path.home() / ".ros/log") + #delete results, logs and bags of previous experiments if they exist + if(Path(path.parents[3].joinpath("bagfiles")).exists()): + shutil.rmtree(path.parents[3].joinpath("bagfiles")) + if(Path(path.parents[3].joinpath("results")).exists()): + shutil.rmtree(path.parents[3].joinpath("results")) + if(Path(Path.home() / ".ros/log").exists()): + shutil.rmtree(Path.home() / ".ros/log") #create the folder where we will record the different bags and the folder where the results pdf will be saved @@ -124,7 +127,9 @@ def translate_and_plot(testname:str, bagfolder:str): atexit.register(clean_process, launch_crazyswarm) time.sleep(1) + print("f8") record_start_and_terminate("figure8", 20, bagfolder) + print("multitrajectory") record_start_and_terminate("multi_trajectory", 80, bagfolder) From 0602401ede7ce83b8ac8f406798512febe7672b9 Mon Sep 17 00:00:00 2001 From: JulienThevenoz Date: Thu, 7 Dec 2023 11:23:06 +0100 Subject: [PATCH 7/7] cleanup --- systemtests/run_test.py | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/systemtests/run_test.py b/systemtests/run_test.py index 3271e4d90..d5f80f554 100755 --- a/systemtests/run_test.py +++ b/systemtests/run_test.py @@ -14,6 +14,7 @@ def clean_process(process:Popen) -> int : '''Kills process and its children on exit if they aren't already terminated (called with atexit). Returns 0 on success, 1 on failure''' + if process.poll() == None: group_id = os.getpgid(process.pid) print(f"cleaning process {group_id}") @@ -22,7 +23,6 @@ def clean_process(process:Popen) -> int : i=0 while i < 10 and process.poll() == None: #in case the process termination is lazy and takes some time, we wait up to 0.5 sec per process if process.poll() != None: - print("yaY") return 0 #if we have a returncode-> it terminated time.sleep(0.05) #if not wait a bit longer if(i == 9): @@ -30,18 +30,18 @@ def clean_process(process:Popen) -> int : return 1 #after 0.5s we stop waiting and consider it did not terminate correctly return 0 else: - print(process.returncode) return 0 #process already terminated -def record_start_and_terminate(testname:str, max_wait:int, bagfolder:str): + +def record_start_and_clean(testname:str, max_wait:int, bagfolder:str): '''Starts recording the /tf topic in a rosbag, starts the test, waits, closes the rosbag and terminate all processes. max_wait is the max amount of time you want to wait before forcefully terminating the test flight script (in case it never finishes correctly). NB the testname must be the name of the crayzflie_examples executable (ie the CLI grammar "ros2 run crazyflie_examples testname" must be valid)''' + index = bagfolder.find("bag_") bagname = bagfolder[index:] - - src = "source " + bagfolder[:index-9] + "install/setup.bash" # -> "source /home/github/actions-runner/_work/crazyswarm2/crazyswarm2/ros2_ws/install/setup.bash" + try: command = f"{src} && ros2 bag record -s mcap -o {testname}_{bagname} /tf" record_bag = Popen(command, shell=True, cwd=bagfolder, stderr=PIPE, stdout=True, @@ -53,23 +53,17 @@ def record_start_and_terminate(testname:str, max_wait:int, bagfolder:str): start_new_session=True, text=True, executable="/bin/bash") atexit.register(clean_process, start_flight_test) - - # if start_flight_test.returncode == 0: #if the flight test script finishes before max_wait, we still want to - # raise TimeoutExpired(command, max_wait) start_flight_test.wait(timeout=max_wait) #raise Timeoutexpired after max_wait seconds if start_flight_test didn't finish by itself clean_process(start_flight_test) clean_process(record_bag) - print("normal finish") except TimeoutExpired: #if max_wait is exceeded clean_process(start_flight_test) clean_process(record_bag) - print("timeout finish") except KeyboardInterrupt: #if drone crashes, user can ^C to skip the waiting clean_process(start_flight_test) clean_process(record_bag) - print("keyboard interrupt finish") #if something went wrong with the bash command lines in Popen, print the error if record_bag.stderr != None: @@ -78,9 +72,9 @@ def record_start_and_terminate(testname:str, max_wait:int, bagfolder:str): print(testname," start_flight flight stderr: ", start_flight_test.stderr.readlines()) - def translate_and_plot(testname:str, bagfolder:str): '''Translates rosbag .mcap format to .csv, then uses that csv to plot a pdf ''' + index = bagfolder.find("bag_") bagname = bagfolder[index:] # NB : the mcap filename is almost the same as the folder name but has _0 at the end @@ -100,6 +94,7 @@ def translate_and_plot(testname:str, bagfolder:str): plotter.create_figures(test_file, rosbag_csv, output_pdf) #plot the data + if __name__ == "__main__": path = Path(__file__) #Path(__file__) in this case "/home/github/actions-runner/_work/crazyswarm2/crazyswarm2/ros2_ws/src/crazyswarm2/systemtests/newsub.py" ; path.parents[0]=.../systemstests @@ -112,7 +107,6 @@ def translate_and_plot(testname:str, bagfolder:str): if(Path(Path.home() / ".ros/log").exists()): shutil.rmtree(Path.home() / ".ros/log") - #create the folder where we will record the different bags and the folder where the results pdf will be saved now = datetime.now().strftime('%d_%m_%Y-%H_%M_%S') bagfolder = str((path.parents[3].joinpath(f"bagfiles/bag_" + now))) # /home/github/actions-runner/_work/crazyswarm2/crazyswarm2/ros2_ws/bagfiles/bag_d_m_Y-H_M_S @@ -124,14 +118,11 @@ def translate_and_plot(testname:str, bagfolder:str): command = f"{src} && ros2 launch crazyflie launch.py" launch_crazyswarm = Popen(command, shell=True, stderr=True, stdout=True, text=True, start_new_session=True, executable="/bin/bash") - atexit.register(clean_process, launch_crazyswarm) + atexit.register(clean_process, launch_crazyswarm) #atexit helps us to make sure processes are cleaned even if script exits unexpectedly time.sleep(1) - print("f8") - record_start_and_terminate("figure8", 20, bagfolder) - print("multitrajectory") - record_start_and_terminate("multi_trajectory", 80, bagfolder) - + record_start_and_clean("figure8", 20, bagfolder) + record_start_and_clean("multi_trajectory", 80, bagfolder) clean_process(launch_crazyswarm) #kill crazyswarm and all of its child processes