fix: test and fix error handling in open_browser #282
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The original implementation requires that commands were executed in the order they were declared, which has worked fine so far. However, when testing some of our threaded behaviour, this was not a valid assumption. This refactors the fixture to add support for setting `run.concurrent = True`, which will match *any* specified command, rather than the next one. The default behaviour is still to require them in order. As an implementation detail, it meant switching away from deque to list made sense, as we want to now be able to remove from anywhere in the list. Note that tests using this new functionality will be in next commit.
The call to `webbrowser.open` calls a subprocess, which will fail as we've not called run.expect for it. However, this error was hidden as test exits quickly enough that it doesn't appear, unless its slow for some reason. This change mocks the call to open_browser, and checks that it is actually called.
We'd previously cheated by passing `--no-browser` to the jupyter call, which avoided testing it. However, this was lazy, and left that code path uncovered. So, here we fully mock/test that flow as well, by: - mocking/testing utils.open_browser - using the new run.concurrent support to mock/assert that the relevant calls are happening.
Switch dyanmically at call time rather than at import time. This allows us to more easily enable debug logs in tests. Also, a small UX tweak - always print a message to the user if we cannot open browser, rather than failing silently
This wasn't covered before, and had some issues. This adds coverate for timeout and error scenarios, and fixes an error found by these tests.
bloodearnest
changed the title
remlapmot
reviewed
Oct 22, 2024
We can't easily pass the cli args to threads, so we've used a DEBUG env var for debugging threaded code. This simplifies all debug output behind the --debug flag.
evansd
approved these changes
Oct 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes an ugly message for a user when something didn't work right.
There were two errors:
responseThese were not covered by tests, as it was awkward to do. But its caused
breakage, so this PR grasps the nettle and adds coverage for:
open_browsererror casesThis required some refactoring of the SubprocessRunFixture, and some other
clean ups.
As a result, test coverage went up by 6%.