Catch a syntax error when parsing an input file with the --stay-connected flag active #126
Conversation
|
I just moved the menu into a function so that I can call it from an except block. Please let me know if there is a cleaner way to do this that I'm not thinking of. |
|
Hi @shaggysa. This got buried in the backlog. For future reference, I personally don't mind a gentle ping if it has been more than two weeks and it seems like I forgot about something. |
|
I moved it to a separate function, but I am unable to test it with a robot at the moment. The linter is happy, but I can't guarantee that it will work the way I intended. |
|
Apparently I've done something to I made some other changes that I just merged to fix a different issue, but it means we no longer get the |
Ouch... That seems like a pretty big vulnerability... I guess let me know if I need to adjust anything to make the cli catch/report it properly once you have that figured out. |
|
OK, the crash is fixed now (you will need to merge the Because of the other change I made, it will now raise It would also be nice if we could get some unit tests for this. |
|
Ok, I'll see what I can do about fixing it to work with the new mpy-cross. I haven't really messed with unit testing before, but I can give that a shot as well. |
|
Ok, I think I'm catching the error properly now (I was actually able to test with a robot this time). The error seems to always show stdin as the file, which should probably be changed to make it a bit more helpful on multi-file projects. |
|
I added a few tests related to the menu, but it'll probably need some major reworking (any possibly some additions) since I don't have any experience with writing unit tests. The final test I wrote (test_stay_connected_menu_interruptions) appears to be leaking coroutines, but I can't figure out how to fix it. |
|
|
…ecify utf-8 encoding
It looks like the mock menu had to have the side effect of an actual async function for it to return a proper coroutine. When its side effect was hardcoded values, the AsyncMock function was unable to be cancelled properly, causing coroutines to leak.
|
I decided to go ahead and add program-cancelling from the terminal. I decided on a non-signal keypress because I didn't want to intercept a KBI in the case that the "racing" function was not active and I didn't want to worry about double presses cancelling everything. I briefly tried EOF, but it looked like I had to manually re-open stdin every time that EOF was received. I landed on an invisible PromptToolKit app that listens for the letter 'q'. The letter it listens for can be easily modified if desired. This should at least partially address #67. One quirk that I can't seem to figure out is that is printed on every call to hub.download. I didn't touch that function, and I know that the race_keypress function isn't causing it because it doesn't happen on hub.start_user_program (which uses race_keypress) and it does happen on hub.download standalone (which doesn't use race_keypress). |
Sounds great. Can we save that for a separate pull request? One feature is enough for one pull request. 😄 |
shaggysa
force-pushed
the
feature/catch-syntax-error
branch
from
293faba to
45fed26
Compare
|
Alright, fair enough. I moved that to a separate branch for the time being. It looks like the random printing in the hub.download function is still an issue in the reverted timeline. shortdemo double-prints "{'pybricks.tools'}", longdemo double-prints "set()", sockfw dobule-prints "{'utime'}", module1 double-prints "{pybricks.parameters}", etc. The printing occurs prior to the loading bar starting. |
|
Ok, the master branch seems to have the exact same issue, so I think that the mpy-cross update has something to do with it. |
The function that this option calls is not available on hub firmware versions prior to 3.2.0-beta.3, so this test confirms that it is not called when the hub's firmware version is too old.
pybricksdev/cli/__init__.py
Outdated
| if args.stay_connected: | ||
| await self.stay_connected_menu(hub, args) | ||
|
|
||
| case ResponseOptions.RUN_STORED: | ||
| if hub.fw_version < Version("3.2.0-beta.4"): | ||
| print( | ||
| "Running a stored program remotely is only supported in the hub firmware version >= v3.2.0." | ||
| ) | ||
| else: | ||
| await hub.start_user_program() | ||
| await hub._wait_for_user_program_stop() | ||
|
|
||
| case ResponseOptions.CHANGE_TARGET_FILE: | ||
| args.file.close() | ||
| while True: | ||
| try: | ||
| args.file = open( | ||
| await hub.race_disconnect( | ||
| hub.race_power_button_press( | ||
| questionary.path( | ||
| "What file would you like to use?" | ||
| ).ask_async() | ||
| ) | ||
| ) | ||
| ) | ||
| break | ||
| except FileNotFoundError: | ||
| print("The file was not found. Please try again.") | ||
| # send the new target file to the hub | ||
| with _get_script_path(args.file) as script_path: | ||
| await hub.download(script_path) | ||
|
|
||
| case _: | ||
| return | ||
|
|
||
| except HubPowerButtonPressedError: | ||
| # This means the user pressed the button on the hub to re-start the | ||
| # current program, so the menu was canceled and we are now printing | ||
| # the hub stdout until the user program ends on the hub. | ||
| try: | ||
| await hub._wait_for_power_button_release() | ||
| await hub._wait_for_user_program_stop() | ||
|
|
||
| except HubDisconnectError: | ||
| hub = await reconnect_hub() | ||
|
|
||
| except HubDisconnectError: | ||
| # let terminal cool off before making a new prompt | ||
| await asyncio.sleep(0.3) | ||
| hub = await reconnect_hub() | ||
| except subprocess.CalledProcessError as e: | ||
| print() | ||
| print("A syntax error occurred while parsing your program:") | ||
| print(e.stderr.decode()) | ||
| if args.stay_connected: | ||
| await self.stay_connected_menu(hub, args) |
There was a problem hiding this comment.
| if args.stay_connected: | |
| await self.stay_connected_menu(hub, args) | |
| case ResponseOptions.RUN_STORED: | |
| if hub.fw_version < Version("3.2.0-beta.4"): | |
| print( | |
| "Running a stored program remotely is only supported in the hub firmware version >= v3.2.0." | |
| ) | |
| else: | |
| await hub.start_user_program() | |
| await hub._wait_for_user_program_stop() | |
| case ResponseOptions.CHANGE_TARGET_FILE: | |
| args.file.close() | |
| while True: | |
| try: | |
| args.file = open( | |
| await hub.race_disconnect( | |
| hub.race_power_button_press( | |
| questionary.path( | |
| "What file would you like to use?" | |
| ).ask_async() | |
| ) | |
| ) | |
| ) | |
| break | |
| except FileNotFoundError: | |
| print("The file was not found. Please try again.") | |
| # send the new target file to the hub | |
| with _get_script_path(args.file) as script_path: | |
| await hub.download(script_path) | |
| case _: | |
| return | |
| except HubPowerButtonPressedError: | |
| # This means the user pressed the button on the hub to re-start the | |
| # current program, so the menu was canceled and we are now printing | |
| # the hub stdout until the user program ends on the hub. | |
| try: | |
| await hub._wait_for_power_button_release() | |
| await hub._wait_for_user_program_stop() | |
| except HubDisconnectError: | |
| hub = await reconnect_hub() | |
| except HubDisconnectError: | |
| # let terminal cool off before making a new prompt | |
| await asyncio.sleep(0.3) | |
| hub = await reconnect_hub() | |
| except subprocess.CalledProcessError as e: | |
| print() | |
| print("A syntax error occurred while parsing your program:") | |
| print(e.stderr.decode()) | |
| if args.stay_connected: | |
| await self.stay_connected_menu(hub, args) | |
| except subprocess.CalledProcessError as e: | |
| print() | |
| print("A syntax error occurred while parsing your program:") | |
| print(e.stderr.decode()) | |
| pass | |
| if args.stay_connected: | |
| await self.stay_connected_menu(hub, args) |
This looks a bit odd with calling stay_connected_menu() twice. We should be able to rearrange it to something like this.
We just need to be careful about the scope of with _get_script_path().
| """Test that the stay connected menu is called upon a syntax error when the appropriate flag is active.""" | ||
|
|
||
| # Create a mock hub | ||
| mock_hub = AsyncMock() |
There was a problem hiding this comment.
I would rather avoid mocks when we can. It should be trivial to create a file with a syntax error to raise this error.
The only time we really need mocks is for hardware that can't be tested on CI, like Bluetooth and USB. Otherwise, a lot of the code isn't getting tested.
|
I think I got most of what you suggested integrated in. One thing to note is that the menu gets called even on a KBI, which we may or may not want. Please let me know if there is anything else you want adjusted. |
2 participants
Most errors are caught by the robot after the program has been sent, but a few will abort the entire program.
While this used to be fine, it can be annoying when the --stay-connected flag is active.
Currently, this only works after the program has been successfully sent once, but I'm open to applying it more broadly. However, getting that to work would probably require a small refactor.