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

Remove a deadcode - the error variable will never be an AFError #22398

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

crazytonyli
Copy link
Contributor

Media.error is an NSError instance. The casting here error as? AFError always fails.

Tests

I didn't do any testing, because I'm pretty sure the deleted code is deadcode—it never runs in the app.

Second, I have verified the chance that the error is originally an AFError is pretty low: 1 out of 1627. So, it'd be low impact if go one step further and stop checking AFError.

The apps send events to Sentry when hasMissingFileError (which is where the deleted code resides) is true. All of these events are associated with this Sentry issue #15096.

guard let mediaError = media.error,
media.hasMissingFileError else {
return
}
self.cancelUploadAndDeleteMedia(media)
WordPressAppDelegate.crashLogging?.logMessage("Deleting a media object that's failed to upload because of a missing local file. \(mediaError)")

I have grabbed all 1627 events from the Sentry issue and find only one 'AFError' occurrence.

Regression Notes

  1. Potential unintended areas of impact
    None.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    None. See the "Tests" section for reasons.

  3. What automated tests I added (or what prevented me from doing so)
    None.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist: N/A

@crazytonyli crazytonyli added this to the 24.1 milestone Jan 15, 2024
@crazytonyli crazytonyli requested a review from mokagio January 15, 2024 21:31
@crazytonyli crazytonyli self-assigned this Jan 15, 2024
@wpmobilebot
Copy link
Contributor

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr22398-9b48b74
Version24.0
Bundle IDorg.wordpress.alpha
Commit9b48b74
App Center BuildWPiOS - One-Offs #8447
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr22398-9b48b74
Version24.0
Bundle IDcom.jetpack.alpha
Commit9b48b74
App Center Buildjetpack-installable-builds #7470
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@crazytonyli
Copy link
Contributor Author

@iangmaia FYI, when I opened this PR, the dangermattic GHA says "it's cancelled". And I have to go to the GHA and re-run the job. Not sure if this is a known issue.

@crazytonyli crazytonyli enabled auto-merge January 15, 2024 22:27
Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

I have grabbed all 1627 events from the Sentry issue and find only one 'AFError' occurrence.

A post on how you did this would be interesting. Nudge nudge.

Media.error is an NSError instance. The casting here error as? AFError always fails.

The fact that Alamofire is on its way out adds to the safety of merging this.

@crazytonyli crazytonyli merged commit d7240e1 into trunk Jan 16, 2024
23 of 26 checks passed
@crazytonyli crazytonyli deleted the decouple-from-alamofire-aferror-0 branch January 16, 2024 04:25
@iangmaia
Copy link
Contributor

iangmaia commented Jan 16, 2024

@iangmaia FYI, when I opened this PR, the dangermattic GHA says "it's cancelled". And I have to go to the GHA and re-run the job. Not sure if this is a known issue.

Hi @crazytonyli, thanks for the ping. Yeah, I've also noticed it -- I've configured this GHA to cancel in progress executions if there's a new one, and that apparently triggers a notification for each job that has been cancelled. There's no need to re-run it manually as there's always a new job that will run when the former one is cancelled.
This happens because when you're working on a PR, you might do all sorts of things (add label, milestone, etc) that will trigger a job, so if it starts running and gets cancelled, GitHub will trigger this notification. There's this discussion about the notifications aspect on GH side.

I've been thinking about how to deal with it -- perhaps we'd be ok by not cancelling in-progress jobs, or grouping their parallel execution in a different way (I currently group them by branch). I'll continue looking into it.

@crazytonyli
Copy link
Contributor Author

There's no need to re-run it manually as there's always a new job that will run when the former one is cancelled.

I had to re-run it because the PR was marked as failure for a while. I didn't see a re-run job then. And it appears there are only two runs: the cancelled one and the re-run one.

Screenshot 2024-01-17 at 6 38 19 AM

@crazytonyli
Copy link
Contributor Author

A post on how you did this would be interesting.

@mokagio I just wrote a simple ruby script that calls an Sentry API to iterate all the associated events in the issue. I don't feel like it's P2-worthy 🥲

@mokagio
Copy link
Contributor

mokagio commented Jan 17, 2024

I feel like it is 😄

Hey all, I wrote a simple Ruby script that calls a Sentry API to check for occurrences of a given error among Sentry events.

I used it in #22398 to gather data showing it was safe to remove handling for AFError in a part of Jetpack and WordPress iOS.

Here's the code:

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

Successfully merging this pull request may close these issues.

4 participants