Add test coverage for Mastodon::AccountsCLI

[danielmbrasil]

This PR adds test coverage for Mastodon::AccountsCLI

Sorry for the huge PR, I should've split it in two at least. But I tried making commits as organized as possible to help with the review.

Methods covered:

• Add tests for Mastodon::AccountsCLI#rotate
• Add tests for Mastodon::AccountsCLI#create
• Add tests for Mastodon::AccountsCLI#modify
• Add tests for Mastodon::AccountsCLI#delete
• Add tests for Mastodon::AccountsCLI#merge
• Add tests for Mastodon::AccountsCLI#fix_duplicates
• Add tests for Mastodon::AccountsCLI#backup
• Add tests for Mastodon::AccountsCLI#cull
• Add tests for Mastodon::AccountsCLI#refresh
• Add tests for Mastodon::AccountsCLI#follow
• Add tests for Mastodon::AccountsCLI#unfollow
• Add tests for Mastodon::AccountsCLI#reset_relantionships
• Add tests for Mastodon::AccountsCLI#approve
• Add tests for Mastodon::AccountsCLI#prune
• Add tests for Mastodon::AccountsCLI#migrate

Coverage: 98.94%

[danielmbrasil]

While trying to write tests for the --reattach --force options I might have found a bug (#24540).

[ClearlyClaire]

@ClearlyClaire what do you think? I'd appreciate any suggestions on how to improve the tests.

Just a small inline nitpick but otherwise it looks great overall!

[ClearlyClaire]

While trying to write tests for the --reattach --force options I might have found a bug (#24540).

Yes, that's a bug, good catch.

Another thing I noticed is that the test failures in CI seem to omit the actual error. I wonder if it's because of not capturing or silencing the output of the Thor commands.

[danielmbrasil]

Another thing I noticed is that the test failures in CI seem to omit the actual error. I wonder if it's because of not capturing or silencing the output of the Thor commands.

Turns out that there were two tests reaching the exit(1) in Mastodon::AccountsCLI#create, and these tests were not expecting an error status code. I've fixed it by adding a raise_error(SystemExist) expectation to those tests.

@ClearlyClaire, is #24557 expected to be merged anytime soon? So that I can include tests for the --reattach --force options and conclude the tests for Mastodon::AccountsCLI#create.

Should this PR only address specs for Mastodon::AccountsCLI? I think it'd be better to address other parts of the CLI in different PRs. What do you think?

[ClearlyClaire]

@ClearlyClaire, is #24557 expected to be merged anytime soon? So that I can include tests for the --reattach --force options and conclude the tests for Mastodon::AccountsCLI#create.

I cannot say, someone else on the team needs to review it.

Should this PR only address specs for Mastodon::AccountsCLI? I think it'd be better to address other parts of the CLI in different PRs. What do you think?

I think both are fine but I'd prefer individual PRs.

[danielmbrasil]

Hi @ClearlyClaire, could you help me with this?

Some methods (e.g. #follow and #unfollow) are parallelized using the #parallelize_with_progress method. The latter calls #reset_connection_pools!, which establishes a new connection with the database. Since each example is run within a database transaction, when a new connection is established the current transaction seems to be rolled back and a new one is created. Thus, the data generated in my example's setup is reset and the example fails.

I'm having problems trying to work around that. I did some research and I found the following options:

1. Set use_transactional_tests to false for each example and then manually clean the database with an after hook.
2. Use the Database Cleaner gem.

The first one sounds a bit odd and I'm not sure if it would actually behave properly. As for the second one, I'm not sure about what side effects it could cause to add a new gem to the project.

What do you think? Do you have any suggestions on how I could test these methods?

[ClearlyClaire]

Oh… that is annoying… this is not ideal, but I think one acceptable short-term solution way would be to stub parallelize_with_progress that does not actually runs things in parallel nor resets the connection.

• Set use_transactional_tests to false for each example and then manually clean the database with an after hook.

• Use the Database Cleaner gem.

I'm under the “second option” would still rely on the first one?

[danielmbrasil]

I think one acceptable short-term solution way would be to stub parallelize_with_progress

I've tried that by stubbing #reset_connection_pools!, and it works. I think I could go with that until a better option is found.

I'm under the “second option” would still rely on the first one?

I did some more research to understand better how this gem works and I came across this. That solution seems to fit this case. However, I don't know if replacing use_transactional_fixtures with Database Cleaner's transaction strategy would break existing tests. I could try to implement that and see how it goes.

@ClearlyClaire what do you think?

[ClearlyClaire]

I would stick with #reset_connection_pools! if it works, that enables testing most of the code with minimal changes.