fix: NoteEditor flaky test
Purpose / Description
The NPE was caused due to the getCurrentMultimediaEditableNote method being called when showing the bottom sheet but to test the bottom sheet we dont need that, so separating the logic to show the media sheet and then testing it.
Fixes
- Fixes #16886
How Has This Been Tested?
Locally tested, test successful
Checklist
Please, go through these checks before submitting the PR.
- [x] You have a descriptive commit message with a short title (first line, max 50 chars).
- [x] You have commented your code, particularly in hard-to-understand areas
- [x] You have performed a self-review of your own code
- [ ] UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
- [ ] UI Changes: You have tested your change using the Google Accessibility Scanner
You can run a batch on your fork if you enable actions, then run the unit test workflow manually with os windows-latest and a large count (maybe 30 ?)
This is what I think of as a "test fix" instead of a "code fix", that is, you have done the equivalent of "hey, if this thing is null let's just pretend it isn't important and move on".
You haven't done an explicit null check + ignore, no, but splitting the logic in to two pieces so you can ignore the piece that showed up null in the flaky test is the same
There was a fix annotation that specifically mentions to fix the !!s and I think that's the real thing to dig in to
https://github.com/ankidroid/Anki-Android/blob/f0e5c32adabb80844e3af5fa85f963f8ed76e60c/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt#L1584-L1591
Here, we want to test the BottomSheet no the MultimediaEditableNote so it is better to separate the logic no?
Separate things as you like, but the PR wouldn't exist except our flaky test runner found a problem. The problem should be probed in a test and root cause fixed, or the PR doesn't really serve a purpose other than to avoid the problem (in testing! users can still hit it...) via convenient refactor
Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically
@david-allison I made changes to the method return non null value also introduced a get val, would like you to see the second commit and check if this works
@criticalAY with apologies, I'm in a period of low availability and may not get to this immediately. Please re-ping on Monday if another maintainer hasn't got this over the finish line