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

bugs test parameter is outdated and Synapse-specific #636

Closed
anoadragon453 opened this issue Jul 3, 2019 · 5 comments
Closed

bugs test parameter is outdated and Synapse-specific #636

anoadragon453 opened this issue Jul 3, 2019 · 5 comments

Comments

@anoadragon453
Copy link
Member

anoadragon453 commented Jul 3, 2019

Dendrite relies on having a whitelist of tests that it can say it's currently passing. Each time a PR allows more tests to pass, the PR-ee has to add more tests to the testfile. We have a script which will tell the user to do this if a test is passing that we expected to fail.

There's some functionality in Sytest that allows you to put a bug parameter into a test, seen in a few places, but notably

test "Enabling an unknown default rule fails with 404",
requires => [ local_user_fixture() ],
bug => "SYN-676",
and
test "Should reject keys claiming to belong to a different user",
requires => [ $fixture ],
bug => "synapse#1396",

Dendrite currently passes these tests, but the CI complains that we should be marking them as whitelisted. They are being marked as whitelisted though. While whitelisting should prevent a test from being marked as expected fail, the bug parameter still does so. The code for that is here:

$params{expect_fail}++ if $params{bug} and not $FIXED_BUGS{ $params{bug} };

These bugs are specific to Synapse, and should probably be replaced with a // TODO (Synapse): instead. Alternatively, we could create a bugs parameter that's specific to each homeserver, but I feel that's a decent chunk of work and ups the barrier of entry of adding support for more homeservers.

@anoadragon453
Copy link
Member Author

@richvdh and I have come up with a plan to remove the bug parameter and instead replicate the functionality using test blacklists in Synapse.

@anoadragon453
Copy link
Member Author

This is fixed by:

matrix-org/synapse#5611
#638

@Cnly
Copy link
Contributor

Cnly commented Jul 4, 2019

The PR didn't close this issue though...

@anoadragon453
Copy link
Member Author

Indeed, oddly.

@richvdh
Copy link
Member

richvdh commented Jul 5, 2019

(it won't be closed until the PR hits the default branch (master))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants