convenient_test_manager_dart never exits on failure

[Tomburgs]

Describe the bug When a test fails (intentionally asserting something that does not exist), convenient_test_manager_dart binary never exits.

It successfully logs the failure

| 2024-02-26T16:29:06.757567Z|debug|ReportHandlerService|handleReportLogEntry called
| 2024-02-26T16:29:06.759378Z|debug|ReportHandlerService|Message: 2024-02-26T16:29:06.737419Z|info|LogHandle|🔵 (#2227232528866880, update) ASSERT after 21 retries with 4015 milliseconds, when {widgets with text "this does not exist, so you should fail"} matches {exactly one matching candidate} nullExpected: exactly
one matching candidate
| 2024-02-26T16:29:06.759648Z|debug|ReportHandlerService|Message:   Actual: _TextWidgetFinder:<Found 0 widgets with text "this does not exist, so you should fail": []>
| 2024-02-26T16:29:06.759866Z|debug|ReportHandlerService|Message:    Which: means none were found but one was expected
| 2024-02-26T16:29:06.760087Z|debug|ReportHandlerService|Message: Extra info for matched elements:
| 2024-02-26T16:29:06.760318Z|debug|ReportHandlerService|Message:  #0      fail (package:matcher/src/expect/expect.dart:149:31)
| 2024-02-26T16:29:06.760558Z|debug|ReportHandlerService|Message: #1      _expect (package:matcher/src/expect/expect.dart:144:3)
| 2024-02-26T16:29:06.760946Z|debug|ReportHandlerService|Message: #2      expect (package:matcher/src/expect/expect.dart:56:3)
| 2024-02-26T16:29:06.761318Z|debug|ReportHandlerService|Message: #3      expect (package:flutter_test/src/widget_tester.dart:458:18)
| 2024-02-26T16:29:06.761560Z|debug|ReportHandlerService|Message: #4      _expectWithRetry (package:convenient_test_dev/src/functions/command.dart:92:9)
| 2024-02-26T16:29:06.761767Z|debug|ReportHandlerService|Message: <asynchronous suspension>
| 2024-02-26T16:29:06.761949Z|debug|ReportHandlerService|Message: #5      ExtTCommand.should (package:convenient_test_dev/src/functions/command.dart:32:5)

After which it never exits, but periodically we get these logs:

| 2024-02-26T16:35:43.181899Z|info|main|monitorWorkerAvailable check
| 🚀 [STATUS] [7:00] [1x completeFailureOrError] [Test=register]
| 🚀 [STATUS] [7:02] [1x completeFailureOrError] [Test=register]
| 🚀 [STATUS] [7:04] [1x completeFailureOrError] [Test=register]

To Reproduce We're running the integration tests in GitHub Actions CI. Essentially we run it like so:

- name: Run tests
  run: |
    flutter run $APP_DIR/integration_test/main_test.dart \
      --host-vmservice-port 9753 \
      --disable-service-auth-codes \
      --dart-define CONVENIENT_TEST_APP_CODE_DIR=$APP_DIR \
      -d ${{ steps.device.outputs.udid }} & # note the ampersand, causing it to be executed asynchronously.
    # Run test manager
    cd /tmp/flutter_convenient_test/packages/convenient_test_manager_dart
    dart run convenient_test_manager_dart

all we do in our test is asserting something that should not exist, like so

await find.text('this does not exist, so you should fail').should(findsOneWidget);

Expected behavior It should exit as soon as the test fails, or it should be possible to configure it so that it exits as soon as it fails. Perhaps should have optional retry logic?

Desktop (please complete the following information):

• OS: running on MacOS 14.0 using act: act --workflows=.github/workflows/integration_tests.yaml -P macos-latest=-self-hosted

Smartphone:

• Device: iPhone 12 Simulator

Additional context Perhaps I'm reading the code wrong here, but it seems like the dart manager binary only listens for WorkerSuperRunStatus.testAllDone for it to exit which is only the state if seenTearDownAll is true. However, if the test errors (runnerError), then this never gets invoked.

[fzyzcjy]

Hmm looks like a bug and your analysis seems pretty reasonable, feel free to PR to fix it!

[Tomburgs]

I've looked into this and I'm not sure what the best way to resolve this is. Should on error a tear down event be made, or would it make more sense for the dart manager to instead look at the state and consider completed as signal to exit the process?

Don't think I'm well enough versed in the codebase/project to make a PR for this unfortunately 🙁

[fzyzcjy]

No worries, then I will check and fix it (hopefully within a week or two)

[Tomburgs]

Any update on this? I might have some time to help patch this if you would be able to give some high level overview of how this should be fixed. I was looking into it and was not 100p sure where the specific signals (error, test finished, etc.) come from and I'm not sure if there is any documentation on this.

I was also thinking if it's possible to just in the convenient_test_manager_dart listen to all the signals and then just respond correspondingly to the ones which should trigger an exit?

[fzyzcjy]

I am a bit busy with https://github.com/fzyzcjy/flutter_rust_bridge and other things, and looking forward to your PR!

give some high level overview of how this should be fixed I was also thinking if it's possible to just in the convenient_test_manager_dart listen to all the signals and then just respond correspondingly to the ones which should trigger an exit?

Sure! I will reply a bit later (hopefully within 2 hours)

[fzyzcjy]

I briefly glanced through it, and here are some thoughts:

• Maybe we should fix bug such that superRunStatus == WorkerSuperRunStatus.testAllDone when all tests ended (whether succeed or fail).
• Suppose you are not using isolation mode, then the _WorkerSuperRunControllerIntegrationTestClassicalMode may be interesting.
• We may add some prints here and there to track how those variables change. For example, one way is to print when _WorkerSuperRunControllerIntegrationTestClassicalMode.handleTearDownAll is called. It seems that it should be called when all tests end (no matter succeed or fail). If it is not called, for example, we can trace up to see whether the grpc server (inside convenient_test_manager_dart) receives it, and if not, whether the grpc client (the convenient_test_dev running on your android simulator) sends it, etc.

I was also thinking if it's possible to just in the convenient_test_manager_dart listen to all the signals and then just respond correspondingly to the ones which should trigger an exit?

Usually testers will wait until all test finish, even if some of them fail, thus I am not very sure whether "exit when seeing first failure" is the best practice or not.

[Tomburgs]

Thanks, that makes sense. One of the immediate things that come to mind is that we need to be able to differentiate between success and failure (i.e exit code 0 or 1) so that CI would be able to report if the tests passed or not.

I'm thinking two things:

1. New value in ResolvedExecutionFilterProto of failed (or similar).
2. WorkerSuperRunStatus needs a new value of testsFailed (or similar).

What I'm not sure about is, and perhaps you could clarify:

1. Where tearDownAll is sent from.
2. Whether it should be a new message instead of ResolvedExecutionFilterProto, as it seems like it's used for other things as well.

[fzyzcjy]

One of the immediate things that come to mind is that we need to be able to differentiate between success and failure (i.e exit code 0 or 1) so that CI would be able to report if the tests passed or not. Whether it should be a new message instead of ResolvedExecutionFilterProto, as it seems like it's used for other things as well.

Sure! I guess it can be done after we fix the bug that it does not exit.

Where tearDownAll is sent from.

From the client (the code running in android simulator) using convenient_test_dev. One way to debug is that, create a successful test (thus it correctly send events and exits), and add some prints to see what is going on.

[Tomburgs]

After looking into this more, it seems like it was our fault from the beginning. Because we were overwriting FlutterError.onError in our app (without invoking the original value) it breaks the tests. Seems like if we remove it from our app the dart manager exits correctly.

I'd say feel free to close this, but I wonder if there is a way around this that would make it easier for future users? I imagine setting FlutterError.onError is a fairly common occurrence and likely in tests not even necessary.

[fzyzcjy]

Happy to see the cause is found!

without invoking the original value but I wonder if there is a way around this that would make it easier for future users? I imagine setting FlutterError.onError is a fairly common occurrence and likely in tests not even necessary.

Invoking the original function may solve the problem. Curious about your scenarios - why the original function should not be called?

One way may be that, let flutter_convenient_test override onError after your custom override. Then flutter_convenient_test will invoke its logic + invoke your function. A simplest way to implement this is to extract the "flutter_convenient_test's setup on error" into a separate public function, and allow users to call it manually.