Anki-Android icon indicating copy to clipboard operation
Anki-Android copied to clipboard

fix: NoteEditor flaky test

Open criticalAY opened this issue 1 year ago • 4 comments

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

criticalAY avatar Aug 17 '24 04:08 criticalAY

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 ?)

image

mikehardy avatar Aug 17 '24 07:08 mikehardy

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

mikehardy avatar Aug 17 '24 16:08 mikehardy

Here, we want to test the BottomSheet no the MultimediaEditableNote so it is better to separate the logic no?

criticalAY avatar Aug 18 '24 06:08 criticalAY

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

mikehardy avatar Aug 19 '24 14:08 mikehardy

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

github-actions[bot] avatar Sep 02 '24 14:09 github-actions[bot]

@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 avatar Sep 06 '24 11:09 criticalAY

@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

david-allison avatar Sep 07 '24 01:09 david-allison