-
-
Notifications
You must be signed in to change notification settings - Fork 389
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add test mode to run
and dev
#962
Conversation
For Android, the easiest way I can think of to set the entry point is for Briefcase to create a resource file, e.g. The name of the file isn't significant: all files in the |
…ys update to test state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any large concerns.
The necessary configuration is a bit opaque to me; will the default templates be updated to perhaps provide a usable example of implementing tests?
I don't know if this would have changed things but Rich was already recording all the captured output. The same Rich API we use to create the log file can be used to obtain a copy of past output.
I haven't been able to get my test suite to run on Android but I'm presuming that user error for now.
Finally, I'm particularly disappointed at all the missed opportunities to use the walrus operator in this PR ;)
try: | ||
# Set up the log stream | ||
kwargs = self._prepare_log_stream(app=app, test_mode=test_mode) | ||
|
||
# Start the app in a way that lets us stream the logs | ||
log_popen = self.tools.subprocess.Popen( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To help draw the difference from macOS, I'd rename all these Popen
objects from log_popen
to app_popen
. It gets confusing to me what the Popen
objects really represent in the different codepaths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This same change also probably makes sense for flatpak and windows.
src/briefcase/commands/run.py
Outdated
# Look for the success/failure conditions in the tail | ||
if self.failure_filter and self.failure_filter(tail): | ||
self.success = False | ||
self.log_popen.send_signal(signal.SIGINT) | ||
elif self.success_filter and self.success_filter(tail): | ||
self.success = True | ||
self.log_popen.send_signal(signal.SIGINT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of sending SIGINT
here? Why shouldn't the process finish normally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Primarily because on iOS and Android, it won't ever finish. iOS and Android apps don't ever truly exit - it's assumed that the app will start, and will persist indefinitely; on macOS, the log stream will persist.
However, we might be able to use the stop_func
in the Android/iOS case; we're already using the stop func on the macOS case. Let me dig into this a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I taken another look at this, and I think I've found an alternate approach. Instead of sending SIGINT, we can tell the streamer to stop. The process will then fall back on the normal subprocess.cleanup()
. We'll still potentially be terminating the process, but the logic is all contained in the subprocess module.
In the process of working this out, I was also able to build a stop_func
implementation for iOS, so we now have a safety mechanism there, too. In normal operation, that safety mechanism won't be triggered; but if someone manages to build an app that fully crashes, the log will terminate along with the app, which brings iOS in alignment with every other platform.
if run_app: | ||
self.logger.info("Starting in dev mode...", prefix=app.app_name) | ||
env = self.get_environment(app) | ||
return self.run_dev_app(app, env, **options) | ||
if test_mode: | ||
self.logger.info( | ||
"Running test suite in dev environment...", prefix=app.app_name | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user experience for briefcase dev --test
for an app that's improperly configured is a little rough.
Do you think some guardrails here and exception masking with more obvious feedback to the user is appropriate?
❯ briefcase dev --test
[testmode] Running test suite in dev environment...
Traceback (most recent call last):
File "/home/russell/.pyenv/versions/3.10.8-debug/lib/python3.10/runpy.py", line 129, in _get_module_details
spec = importlib.util.find_spec(mod_name)
File "/home/russell/.pyenv/versions/3.10.8-debug/lib/python3.10/importlib/util.py", line 94, in find_spec
parent = __import__(parent_name, fromlist=['__path__'])
ModuleNotFoundError: No module named 'tests'
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "/home/russell/.pyenv/versions/3.10.8-debug/lib/python3.10/runpy.py", line 220, in run_module
mod_name, mod_spec, code = _get_module_details(mod_name)
File "/home/russell/.pyenv/versions/3.10.8-debug/lib/python3.10/runpy.py", line 138, in _get_module_details
raise error(msg.format(mod_name, type(ex).__name__, ex)) from ex
ImportError: Error while finding module specification for 'tests.testmode' (ModuleNotFoundError: No module named 'tests')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm - I agree this isn't a great UX. However, there are some limits on what we can do here, because there's functionally no difference between user-space code raising an ImportError
, and the main execution script raising an ImportError
. We could catch the ImportError, and then interrogate to see if the underlying cause is the module that was used to start the app; however, that kind of interrogation makes me really nervous - being "smart" in this kind of way has burned me in the past.
However - I noticed that I'm inadvertently eating the error condition; The mainline essentially ignores BriefcaseTestSuiteError
, assuming the method that raised it has done all the error handling, so when your test suite raises an error, that's not caught as a "couldn't start test suite" error. Catching that error at least gives us the chance to raise a slightly more meaningful "Unable to start test suite 'tests.myapp'" message, and capture the log; test suite success/failure is a "successful" run.
Does that look any better?
That's the idea. Adding a default test suite to the briefcase template is on my todo list for today.
It would be - except for the iOS/Android case. Since those test apps never truly exit, we need to have a streaming log filter to identify the failure conditions. Plus, it gives us the opportunity to strip off some of the excessive logging noise added by the macOS log stream.
Yeah - that needs some hand-holding for now, plus a manual modification to the gradle template. @mhsmith pushed some updates last night which provide a temporary workaround for the manual modifications. If you want a working example, beeware/Python-support-testbed#6 is the best point of reference.
:-P |
I think all of my comments have been addressed, except for this one about the |
@rmartin16 FYI - I've added beeware/briefcase-template#39; this will bootstrap a pytest test suite into a new project. The template contains an option for a unittest suite; for now, you need to manually roll out the template to get that option. We could add the extra question to this PR; but (a) I don't think it hurts to strongly encourage pytest, and (b) we really need the longer term fix in the form of a fix for #392. |
I've had a quick word with @rmartin16 on Discord, and he's given the go-ahead to merge this. We're certain to have tweaks as we fine tune the testing behavior as we gain experience, but it will be easier to handle those as PRs on this base. If nothing else: when beeware/briefcase-template#39 lands, we'll be able to extend CI to run the minimal test suite, which will give us the first proof that Briefcase is always producing an executable that can execute. |
Take 3. See #949 and #961 for the previous 2 attempts.
Adds a
--test
option tobriefcase run
andbriefcase dev
. When in--test
mode:<app_name>
totests.<app_name>
test_sources
test_requires
This approach has the the following advantages:
The downside is that:
Implemented for:
If you specify a
template_branch='testcase2'
at the app level, these templates will be picked up.The most common example of usage will be a pytest suite, where all the test code is contained in a tests folder. This would be configured as:
the code copied in as part of the
test_sources
should contain anapp_name.py
that will start the test suite. This script should contain:Alternatively, a unittest suite with automatic discovery would look like:
And a unittest suite based on explicit code imports would look like:
Other notes:
run --test
will implicitly do a full update and build on every execution. This will resulting the test code and dependencies always being available when running as a test; however, it also means that if you dorun
directly afterrun --test
, the test dependencies will be in the app. Mobile platforms will also end up in a situation where they run as the test app. So - it is advisable to do arun -u
the first time after runningrun --test
.Fixes #973
PR Checklist: