search

matrix-org / sytest

Black-box integration testing for Matrix homeservers
Apache License 2.0
72 stars 55 forks source link

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

Open ShadowJonathan opened 2 years ago

ShadowJonathan commented 2 years ago

https://github.com/matrix-org/sytest/blob/4784b7ed3337962e7938d368d2748953d627d5bf/tests/30rooms/60version_upgrade.pl#L630-L643

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:

https://github.com/matrix-org/sytest/blob/74f593121bcf66193a90a674b20f30a52f5fcb28/run-tests.pl#L846-L856

Finally, $f_test is awaited here:

https://github.com/matrix-org/sytest/blob/74f593121bcf66193a90a674b20f30a52f5fcb28/run-tests.pl#L864-L869

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/matrix-org/sytest/blob/4784b7ed3337962e7938d368d2748953d627d5bf/tests/30rooms/60version_upgrade.pl#L631-L641 won't do anything unless its result is waited on (it's like calling an async function and not awaiting its result)