Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

On Windows, test mode requires an extra line after the summary line #981

Closed
mhsmith opened this issue Nov 28, 2022 · 5 comments · Fixed by #982
Closed

On Windows, test mode requires an extra line after the summary line #981

mhsmith opened this issue Nov 28, 2022 · 5 comments · Fixed by #982
Labels
bug A crash or error in behavior.

Comments

@mhsmith
Copy link
Member

mhsmith commented Nov 28, 2022

Describe the bug
The app in beeware/toga#1687 contains a call to exit the app once the tests are complete. This line should not be necessary, because briefcase run --test should exit once the pytest summary line is detected. But if you comment it out, Briefcase prints the summary line:

======================== 7 passed, 1 skipped in 2.44s =========================

and then hangs until you manually close the window. Only then does it print:

[toga-test] Test suite passed!

and then Briefcase exits.

I suspect this may be something to do with the way the LogFilter sets its success attribute but doesn't check it until the following iteration. Sure enough, if you add a print after pytest.main, the extra line does not appear, but briefcase run --test now exits correctly, and kills the app as well.

Environment:

  • Operating System: Windows
  • Python version: 3.8
  • Software versions:
@mhsmith mhsmith added the bug A crash or error in behavior. label Nov 28, 2022
@freakboy3742
Copy link
Member

This is an interesting edge case. One of the last minute changes to #962 was to remove the explicit call sending SIGINT when the success/failure line was detected, in favour of soft-requesting the end of streaming the first time you process a line when a success criterion has been detected.

This means that the test suite won't terminate itself unless there is at least one additional line after the success criterion.

The test suite will also terminate if the test process itself fails. However, in the case of the GUI tests where an app is being started in the background, the test process won't stop, because it's a running app. So - because the test suite doesn't produce a line of output after the success line, and the main process doesn't stop on its own, the test suite never terminates.

The immediate workaround: Put an additional line of output in the test suite. print("Terminating test app...") would be enough to make this work.

Possible fixes:

  1. Restore the explicit call to emit SIGINT
  2. Modify the return value of the log filter to be a pair of "log value, continue streaming", rather than just "log value". This means the log termination condition can be detected at the same time as the success condition.
  3. Modify a "timeout" mechanism on logs so that if a log doesn't emit in N second, the streamer terminates. This is potentially risk in tests; if a test takes a long time to run, it could terminate early.

I think (2) makes the most sense; I'm not wild about the tuple return value, but I'm not sure I see a cleaner option, and a tuple isn't that bad as a log filter return value.

@rmartin16
Copy link
Member

Maybe a little half-baked....but the streamer could require (or allow) filter_func to be a generator.

@freakboy3742
Copy link
Member

Are you suggesting that streamer would invokes filter_func on a single line of input, and the filter func would then "generate" the processed output - yielding 1 value most of the time, but yielding 2 values (the actual output, then the instruction to terminate) on the last line? I guess that could work, we'd need to use something other than StopIteration to differentiate between "the generator has stopped" and "the streamer should stop". And, in all but 1 invocation (The last one), it would yield a single value - so it feels a bit like a using a sledgehammer to crack a nut.

Alternatively, we'd need to rejigger the whole of the streaming mechanism to make the filter function take control of consuming the log input - which I'm not sure would be any different from making the _stream_output_thread customizable, rather than passing a filter function.

@rmartin16
Copy link
Member

Are you suggesting that streamer would invokes filter_func on a single line of input, and the filter func would then "generate" the processed output - yielding 1 value most of the time, but yielding 2 values (the actual output, then the instruction to terminate) on the last line?

Basically. The motivation for a generator here would be to continue allowing the streamer to be responsible for printing while returning control back to filter_func after it yields its output. The usefulness of generating multiple lines of output from a single input line seems dubious....but it is a simple (at least straightforward) mechanism to allow filter_func to take further action after printing such as raising an arbitrary exception.

Another (even more half-baked :) idea is to allow filter_func to accept a callable that takes the value to print. That way, the streamer could delegate the actual printing to filter_func...

@freakboy3742
Copy link
Member

Thinking about this some more, the other upside to using yield to return output is that the "swallow output" case is "don't yield". While you're correct "multiple lines of output" case isn't going to be common, "zero lines of output" is common.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A crash or error in behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants