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

test(custom-scheduler): stabilize custom scheduler testing #16533

Merged

Conversation

mikehardy
Copy link
Member

@mikehardy mikehardy commented Jun 2, 2024

Purpose / Description

After implementing the reviewer fragment thing in the new reviewer with #16455 the custom scheduler test was failing for me on the command line so my customary / frequent ./gradlew jacocoTestReport did not pass

I believe others did not notice it because:

1- it is not executed in CI because of a Flaky annotation
2- I suppose people developing locally either don't run tests or run them from Android Studio? I use the command line

I need that to pass, so I looked into it, and this or something like this will fix it

Fixes

Approach

1- realize by view hierarchy inspection that the hierarchies are different from the command line and from Android Studio. I have no idea. They are though. So...allow either name for the the layout flip or ease 3 button view resource to match 🤷
2- realize that timing issues are still present (even for me locally) but if we use the otherwise-unused retry rule with a retry of 3, it works every time locally! So I am passing now
3- try to get this working in CI so it doesn't start silently failing again and sending me on a yak shave. Bumping retry to 10 seems to do it? 🙏

Note there was a line-ending issue in initial push that has been peeled off to #16536

How Has This Been Tested?

It's all tests, it is made of tests. But this was a flake, so it has a a higher burden of proof. I'm 9 for 9 running this so I think I actually stabilized it

Learning (optional, can help others)

  • New emulator on new ubuntu runner is pretty quick, default view match timeout is 10s and we don't need 30s anymore
  • we already had a retry rule which was cool
  • view hierarchies can be different between command line and android studio, and I can't even begin to explain that
  • I cannot find a way to run just a single test or regex of tests from the androidTest hierarchy via command line 🤷

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • 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

@mikehardy
Copy link
Member Author

mikehardy commented Jun 2, 2024

(note, this passes over and over again on my fork (4 in a row now, running 5th) - example - https://github.com/mikehardy/Anki-Android/actions/runs/9341737197/job/25709045233 )

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Rejecting due to whitespace changes in commit 1 on ReviewerTest - the entire file is shown as being recreated

Approve after this is fixed, suggestion is a nitpick for clarity

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Jun 3, 2024
@mikehardy
Copy link
Member Author

Yeah the spacing change is ridiculous and will definitely be changed if this passes muster at all:

If this works for everyone else I'll fix up the ridiculous "full file line-ending" change thing that happened on my computer. No idea, and I don't want to shave that yak until there's buy in on this PR. View diff with spacing changes ignored

Seems like it might - with nitpick for clarity 👍 - so will clean this up a bit and repush

without this, `./gradlew jacocoAndroidTestReport` would not work on the command
line, but the tests would work in Android Studio, which is quite bizarre
with appropriate retries it appears to work reliably
@mikehardy mikehardy force-pushed the maybe-stabilize-custom-scheduler-testing branch from 462cd03 to 1384973 Compare June 3, 2024 17:55
@mikehardy mikehardy changed the title WIP test(custom-scheduler): maybe stabilize custom scheduler testing test(custom-scheduler): stabilize custom scheduler testing Jun 3, 2024
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Fantastic, cheers!

@mikehardy
Copy link
Member Author

image

@mikehardy mikehardy added this pull request to the merge queue Jun 3, 2024
@mikehardy
Copy link
Member Author

mikehardy commented Jun 3, 2024

Succeeded first time here as well, and the merge queue will run it again and then it'll run again again on push to main, so 3 more runs to give it a final look and hope it is actually non-flaky

If this starts to flake again - for anyone reading this in the future - just readd the Flaky annotations, do not change any of the other work here as it was all necessary (even the retries!) in order for it to work locally even.

@mikehardy mikehardy added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Author Reply Waiting for a reply from the original author labels Jun 3, 2024
Merged via the queue into ankidroid:main with commit 8d5d3da Jun 3, 2024
8 checks passed
@github-actions github-actions bot added this to the 2.19 release milestone Jun 3, 2024
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Jun 3, 2024
@mikehardy mikehardy deleted the maybe-stabilize-custom-scheduler-testing branch June 3, 2024 18:34
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.

2 participants