#2933. Add MultiStreamController tests. Part 4.

[sgrekhov]

No description provided.

[sgrekhov]

Converted to a draft. onResume test should be updated according to the dart-lang/sdk#56927

[sgrekhov]

Ready for review

[eernstg]

Added a few comments.

[eernstg]

But this is the set-to-null case?

[sgrekhov]

Yes. Updated the description.

[sgrekhov]

Thank you. Updated. Please confirm if we need to remove .onEvent = ...; onEvent = null; tests. I don't think that they do much harm, they simply tests an appropriate statement.

[sgrekhov]

Yes. Updated the description.

[eernstg]

Thanks for the fixes! I think we should take one more iteration, though, on the question about testing variables with or without history.

[sgrekhov]

If we only lines 24-26 then how we will know that the test failed? Therefore removed these tests at all. PTAL.

[eernstg]

I tried to improve my explanation.

[eernstg]

I don't understand why we'd delete the entire test. Here's what I was suggesting:

Suggested change

	controller.onCancel = () {
	Expect.fail("Unexpected onCancel");
	};
	controller.onCancel = null;
	controller.onCancel = null;

The point is simply that I'm worried about introducing a criterion for testing "what happens when the variable x has the value v?" which says that we must test the situation where x had the value 'Hello!' and was then set to null, and we must test the situation where it had the value true and was set to null, and so on (roughly covering whatever is allowed by the type of x).

I just think things are going to explode if we start considering the state history of the variable.

In other words, it matters that we're setting onCancel to null in line 23, but it should not matter which value onCancel had before that (and, in general, we can't afford to start testing "what if this variable has a specific state history?").

Or why would you expect such things to make a difference?

(Yes, we can easily create an example:

class A {
  static const initialValue = 1;
  bool hasHadADifferentValue = false;
  int _x = initialValue;
  int get x => _x;
  set x(int newValue) {
    if (newValue != initialValue) hasHadADifferentValue = true;
    _x = newValue;
  }
  void doit() => print(hasHadADifferentValue ? 'Not normal!' : 'Normal behavior');
}

void main() {
  var a = A();
  a.doit(); // 'Normal behavior'.
  a.x = 100;
  a.x = A.initialValue;
  a.doit(); // 'Not normal!'.
}

and in this example it does matter that a.x has had the value 100 at some point before it gets the value A.initialValue. But we don't expect that kind of history sensitivity for variables in general.)

Clearly, I'm missing something! (Or, alternatively, we shouldn't delete these tests. ;-)

[sgrekhov]

Yes, I understand your concern, but with the suggested change how can we be sure that the tests don't fail? What if after setting onSomething to null some callback is called anyway? We'll newer know about this and the tests will simply always pass. With

    controller.onCancel = () {
      Expect.fail("Unexpected onCancel");
    };

we at least make sure that setting onCancel to null clears the callback immediately, not after some microtask. Wihout it I'd prefer don't have a test which is always pass and only increases tryjobs load. Especiallly taking into account that controller.onCancel = null; doesn't change anything because it already has a null value and we check it in an earlier tests.

[lrhn]

(You can use null as first argument to listen if you don't want to do anything for data events.)

[eernstg]

Thanks! Sounds like that's a case that needs to be tested as well (it might just throw during the invocation of listen, rather than doing nothing on each data event).

[eernstg]

Do we have a test about passing null as the first argument to listen?

[sgrekhov]

Yes, we have. https://github.com/dart-lang/co19/blob/master/LibTest/async/Stream/listen_A02_t01.test.dart. But I've updated this test too for the brevity.

[lrhn]

No need for the controller.close() here. It will be delivered after asyncEnd() has run, so at least for the second stream, the test is already considered completed.

[sgrekhov]

Thank you. Removed.

[eernstg]

I think asyncStart(2) sets up the expectation that asyncEnd() is invoked twice, and the test is not considered to have been completed before that has happened.

Also, the execution of asyncEnd() will adjust a counter. That is, it has no immediate effect other than changing an int (and printing 'unittest-suite-success' when the counter reaches zero, and throwing if the counter would go negative).

So it's not like anything will be preempted by calling asyncEnd() (unless it throws because we haven't maintained the balance).

Another matter is that controller.close() returns a future whose completion is handled as described here, so we might want to await that future in order to see that the behavior is as described. This could be done in several of these tests, or it could be the topic of a separate test (or it might already be covered by some other test).

[lrhn]

Correct on the behavior of asyncStart/asyncEnd.
The thing is that the test is considered done after the second asyncEnd. If nothing has failed before that, the test is successful, irrevocably. In a way, it does preempt the test result.

Anything that happens after that does not affect the test result, so preferably nothing should happen at all. Something throwing after the test is complete is a bug in the test, not a test failure.

In this test, the behavior of controller.close() is not what is tested, so there is no need to call it at all.
If we do want to test it, then the asyncEnd() should be moved to after that test has been performed.

[sgrekhov]

controller.close() was removed from this test, so it should be Ok. asyncEnd() here checks that onResume was really called.

[eernstg]

In a way, it does preempt the test result.

I don't think so, the test runner just checks that the subprocess output includes unittest-suite-success, but it is certainly still possible for the test to proceed, and to succeed or fail based on later actions.

import 'dart:async';
import "package:expect/async_helper.dart";

void main() {
  asyncStart();
  asyncEnd();
  throw "Failed!"; // Causes the test to fail, comment it out and it succeeds.
}

Something throwing after the test is complete is a bug in the test, not a test failure

It isn't quite fair to say that this is what is happening in examples like the one above, or in LibTest/async/MultiStreamController/onResume_A01_t01.dart.

The test will succeed or fail according to the test runner (which is what counts), and there's no difference between failures happening before or after the final invocation of asyncEnd().

What asyncStart(_)/asyncEnd() will do is simply to support behavior that is detected as a test run failure in the case where the stand/end invocation counts differ. This can be used to detect, in particular, that a given piece of code which was expected to run did not actually run.

In this test, the behavior of controller.close() is not what is tested, so there is no need to call it at all.

OK, that's good to know! Perhaps controller.close() should then be deleted from all tests which aren't testing this, and then we should make sure there is one or more tests targeting the effects of invoking controller.close().

If we do want to test it, then the asyncEnd() should be moved to after that test has been performed.

That makes sense, too. It shouldn't matter where it occurs in a piece of code that does not include scheduling points (like await), but it's safer to put it at the end, indicating that all of the given code has been executed when the test terminates.

[eernstg]

Closing this thread. I don't think there's anything substantial to resolve here. ;-)

[sgrekhov]

The thread is closed but I can't find my comment here. Adding just in case.

There was a long discussion about it. See
#2400 (comment)

[eernstg]

Oh, if we're not done then I'll unresolve this thread.

[eernstg]

Just a few extra comments.

[eernstg]

Thanks! Sounds like that's a case that needs to be tested as well (it might just throw during the invocation of listen, rather than doing nothing on each data event).

[eernstg]

I think asyncStart(2) sets up the expectation that asyncEnd() is invoked twice, and the test is not considered to have been completed before that has happened.

Also, the execution of asyncEnd() will adjust a counter. That is, it has no immediate effect other than changing an int (and printing 'unittest-suite-success' when the counter reaches zero, and throwing if the counter would go negative).

So it's not like anything will be preempted by calling asyncEnd() (unless it throws because we haven't maintained the balance).

Another matter is that controller.close() returns a future whose completion is handled as described here, so we might want to await that future in order to see that the behavior is as described. This could be done in several of these tests, or it could be the topic of a separate test (or it might already be covered by some other test).

[sgrekhov]

Readded onSomeEvent = null tests. PTAL

[eernstg]

Looks good, nearly done!

[eernstg]

In a way, it does preempt the test result.

I don't think so, the test runner just checks that the subprocess output includes unittest-suite-success, but it is certainly still possible for the test to proceed, and to succeed or fail based on later actions.

import 'dart:async';
import "package:expect/async_helper.dart";

void main() {
  asyncStart();
  asyncEnd();
  throw "Failed!"; // Causes the test to fail, comment it out and it succeeds.
}

Something throwing after the test is complete is a bug in the test, not a test failure

It isn't quite fair to say that this is what is happening in examples like the one above, or in LibTest/async/MultiStreamController/onResume_A01_t01.dart.

The test will succeed or fail according to the test runner (which is what counts), and there's no difference between failures happening before or after the final invocation of asyncEnd().

What asyncStart(_)/asyncEnd() will do is simply to support behavior that is detected as a test run failure in the case where the stand/end invocation counts differ. This can be used to detect, in particular, that a given piece of code which was expected to run did not actually run.

In this test, the behavior of controller.close() is not what is tested, so there is no need to call it at all.

OK, that's good to know! Perhaps controller.close() should then be deleted from all tests which aren't testing this, and then we should make sure there is one or more tests targeting the effects of invoking controller.close().

If we do want to test it, then the asyncEnd() should be moved to after that test has been performed.

That makes sense, too. It shouldn't matter where it occurs in a piece of code that does not include scheduling points (like await), but it's safer to put it at the end, indicating that all of the given code has been executed when the test terminates.

[eernstg]

Do we have a test about passing null as the first argument to listen?

[sgrekhov]

Updated. PTAL

[sgrekhov]

Yes, we have. https://github.com/dart-lang/co19/blob/master/LibTest/async/Stream/listen_A02_t01.test.dart. But I've updated this test too for the brevity.

[eernstg]

LGTM

[eernstg]

Closing this thread. I don't think there's anything substantial to resolve here. ;-)