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

Add regression test for #4169 #523

Merged
merged 4 commits into from
Nov 14, 2018
Merged

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Nov 9, 2018

Test for matrix-org/synapse#4113

If this fails, it keeps going through the 10 versions which isn't ideal. I tried using the repeat+foreach+while to terminate it but couldn't get it to abort. Feedback welcome on the right runes to make this happen.

@richvdh
Copy link
Member

richvdh commented Nov 9, 2018

I think you can do something like:

      try_repeat( sub {
         matrix_create_key_backup( $user );
      }, foreach => [ 0 .. 10 ], while => sub { $_[0] -> is_done } );

whether it's worth doing so, I couldn't say...

@richvdh
Copy link
Member

richvdh commented Nov 9, 2018

the code lgtm but I'm not reassured by the failures!

@dbkr
Copy link
Member Author

dbkr commented Nov 9, 2018

That's better. Ptal!

test "Can create more than 10 backup versions",
requires => [ $fixture ],

bug => 'synapse/4169',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this does what you want. It marks the test as an "expected failure".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume it only does this while the bug is still open though?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd be wrong :) The string isn't interpreted at all, and certainly doesn't go and check the status of any linked issue (nor is there any consistency in the format of the string)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like you're right though

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@dbkr dbkr merged commit 208a67a into develop Nov 14, 2018
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

Successfully merging this pull request may close these issues.

3 participants