Closed Plastikmensch closed 10 months ago
This needs some more test cases and also adjustment for coming code changes.
I'm afraid squashing this will no longer work...
Would you mind checking out a new branch from the emoji reactions patch and applying your changes to the tests on top of it? You could force-push it to the same branch afterwards.
I'm afraid squashing this will no longer work...
Would you mind checking out a new branch from the emoji reactions patch and applying your changes to the tests on top of it? You could force-push it to the same branch afterwards.
I have to basically redo this anyway, as I changed the specs a lot since opening this. Also some specs fail because of 88cb32e766217dc90c8fea8d876970ae61afafe9 and because the fabricator doesn't initialise custom_emoji with nil.
I mean, we can just initialize custom_emoji
with nil in the fabricator, right? That would fix that one issue.
As for the "we no longer double-check for an emoji's existence before removing a reaction" commit - as the behavior is similar to what Mastodon does on unfavouriting when a post is not favourited to begin with, I think the response should not be tested in regards to "does this reaction actually exist already" anyway.
I mean, we can just initialize
custom_emoji
with nil in the fabricator, right? That would fix that one issue.
Yes.
As for the "we no longer double-check for an emoji's existence before removing a reaction" commit - as the behavior is similar to what Mastodon does on unfavouriting when a post is not favourited to begin with, I think the response should not be tested in regards to "does this reaction actually exist already" anyway.
Should've explained more. The redundant check serves an additional purpose and because that's gone, a specific test case fails by giving an unexpected response. I'm running the test suite on that again, to give more info.
Output of the spec:
1) Api::V1::Statuses::ReactionsController with an oauth token POST #destroy with public status when blocked by its author returns http success
Failure/Error: expect(response).to have_http_status(200)
expected the response to have status code 200 but it was 404
# ./spec/controllers/api/v1/statuses/reactions_controller_spec.rb:98:in `block (5 levels) in <top (required)>'
Progress: |============================================
2) Api::V1::Statuses::ReactionsController with an oauth token POST #destroy with public status when blocked by its author updates the reacted attribute
Failure/Error: expect(user.account.reacted?(status, '👍')).to be false
expected false
got true
# ./spec/controllers/api/v1/statuses/reactions_controller_spec.rb:102:in `block (5 levels) in <top (required)>'
Progress: |============================================
3) Api::V1::Statuses::ReactionsController with an oauth token POST #destroy with public status when blocked by its author returns json with updated attributes
Failure/Error: expect(hash_body[:id]).to eq status.id.to_s
expected: "11044090557811[29](https://github.com/polyamspace/mastodon/actions/runs/5099063120/jobs/9166578519?pr=147#step:10:30)[39](https://github.com/polyamspace/mastodon/actions/runs/5099063120/jobs/9166578519?pr=147#step:10:40)"
got: nil
(compared using ==)
# ./spec/controllers/api/v1/statuses/reactions_controller_spec.rb:108:in `block (5 levels) in <top (required)>'
Progress: |====================================================================|
Just reverting 88cb32e766217dc90c8fea8d876970ae61afafe9 would cause another failure though, as there was some error in it. It should be:
name, domain = params[:id].split('@')
custom_emoji = CustomEmoji.find_by(shortcode: name, domain: domain)
react = current_account.status_reactions.find_by(status_id: params[:status_id], name: name, custom_emoji: custom_emoji)
if react
@status = react.status
UnreactWorker.perform_async(current_account.id, @status.id, params[:id])
else
@status = Status.find(params[:status_id])
authorize @status, :show?
end
I essentially just copied the specs from my custom fork to this branch. My fork is a bit ahead though, so this might include test cases, which should be added at a later date.
I apparently hit a limit for workflows (or they were broken), so I couldn't run the full test suite on these, but the ones which didn't refuse to start all succeeded and I tested them locally.
Feel free to squash