30rooms/60version_upgrade: "$user_type user has push rules copied to upgraded room" does not properly fail on 0 return

[ShadowJonathan]

sytest/tests/30rooms/60version_upgrade.pl

Lines 630 to 643 in 4784b7e

	my $found = 0;
	try_repeat {
	my $to_check = shift;
	my ( $kind, $rule_id ) = @$to_check;
	log_if_fail( "testing $rule_id against $new_room_id" );
	if ( $rule_id eq $new_room_id ) {
	$found = 1;
	}
	} foreach => \@to_check;
	Future->done( $found );

Looking into how tests are returned and checked, it seems that only with a check => subroutine it'll actually check if the value returned is truthy or not:

sytest/run-tests.pl

Lines 846 to 856 in 74f5931

	if( $check ) {
	$f_test = $f_test->then( sub {
	Future->wrap( $check->( @reqs ) )
	})->then( sub {
	my ( $result ) = @_;
	$result or
	die "Test check function failed to return a true value";
	Future->done;
	});
	}

Finally, $f_test is awaited here:

sytest/run-tests.pl

Lines 864 to 869 in 74f5931

	Future->wait_any(
	$f_test,
	delay( $test->timeout // 10 )
	->then_fail( "Timed out waiting for test" )
	)->get;

---

As the above test does not have a check function, the result of 0 seems to be ignored.

I ran into this while converting the test to complement, and dendrite did not pass the test, while it does pass the sytest.

---

I'm not proficient in perl, and the codebase takes odd twists and turns when defining and returning test results like this, so please someone proficient double-check my research, and see if this bug is indeed real.

[squahtx]

I'm not very familiar with perl either. The explanation looks plausible.
PRs to fix things would be very welcome, though if the tests have been ported to complement, we'd like to delete them instead.

[richvdh]

Your interpretation looks correct.

Note that the behaviour of do and check are documented at https://github.com/matrix-org/sytest/blob/develop/DEVELOP.rst#code-blocks, but this difference between a standalone do and a standalone check doesn't seem to be made explicit.

Also worth noting that that the try_repeat block at

sytest/tests/30rooms/60version_upgrade.pl

Lines 631 to 641 in 4784b7e

	try_repeat {
	my $to_check = shift;
	my ( $kind, $rule_id ) = @$to_check;
	log_if_fail( "testing $rule_id against $new_room_id" );
	if ( $rule_id eq $new_room_id ) {
	$found = 1;
	}
	} foreach => \@to_check;

won't do anything unless its result is waited on (it's like calling an async function and not awaiting its result)