Improve systemtests (#378)

* clean processes automatically at exit; replaced custom Waiter class with Popen.wait() function
* cleanup

.github/workflows/systemtests.yml

@@ -2,7 +2,7 @@ name: System Tests
 
 on:
  push:
- branches: [ "feature_systemtests" ]
+ branches: [ "feature-systemtests-better" ]
  # manual trigger
  workflow_dispatch:
 

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,45 +12,58 @@
 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 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}")
+ os.killpg(group_id, signal.SIGTERM)
+ 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:
+ 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:
+ return 0 #process already terminated
 
- 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 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
+
+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"
-
- 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") 
 
- 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")
-
+ 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, 
+ 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)
 
- Waiter(testduration) #wait x seconds for the crazyflie to fly the test
+ 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)
 
- 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)
+
+ except KeyboardInterrupt: #if drone crashes, user can ^C to skip the waiting
+ clean_process(start_flight_test) 
+ clean_process(record_bag)
 
  #if something went wrong with the bash command lines in Popen, print the error
  if record_bag.stderr != None:
@@ -59,9 +72,9 @@ def record_start_and_terminate(testname:str, testduration:int, bagfolder:str):
  print(testname," start_flight flight stderr: ", start_flight_test.stderr.readlines())
 
 
-
 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
@@ -81,15 +94,18 @@ 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__) 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"))
- 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
  now = datetime.now().strftime('%d_%m_%Y-%H_%M_%S')
@@ -102,14 +118,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) #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("multi")
- record_start_and_terminate("multi_trajectory", 80, bagfolder)
+ record_start_and_clean("figure8", 20, bagfolder)
+ record_start_and_clean("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