Update the abort sequence logic to catch exceptions in exit. #224
Conversation
Jasper-Harvey0
requested review from
clint-lawrence,
daniel-montanari and
jcollins1983
|
I haven't looked at the diff, but your explanation makes sense. I realise the existing situation isn't great, but it is it feasible to add a test? |
|
Right so I have some basic tests implemented. They just check for an expected set of calls to the pubsub topics, as well as a finish result from the sequencer. EDIT: I realised we went backwards a little with the tests, so I am in the process of adding back in the mocking stuff. Just need to decide if we keep the pubsub checks too. |
|
Ok, I think I have the tests in a slightly better place. They check for correct pubsub calls, as well as correct calls to the test list / test class functions. I've tried to capture the case of having an error in each of the key test phases. I did have a little bit of trouble coming up with a 'nice' way of doing it, but see what you think. |
|
Anything holding this back? Looks like @jcollins1983 was pretty happy with it, so do we just need the release notes fixed and then merge? |
|
Release notes updated and main has been merged in. I think this is good to go. |
3 participants
The test exit functionality currently does not catch exceptions raised in the exit() function when aborting a sequence early.
This causes the GUI to hang and become unresponsive, meaning you have to force close the window.
The update now catches exceptions raised in the test exit functions and logs them. I have also removed the try / finally block around run_once() as there is already a lot of try except logic in that function, and it just returns when an exception is caught anyway.
I don't know if we want to catch certain errors, instead of "Exception", but I figure something bad has already happened if we are running that code, more errors should just be logged for processing later?