search

wordpress-mobile / AztecEditor-Android

A reusable native Android rich text editor component.
Mozilla Public License 2.0
675 stars 112 forks source link

Android X migration #825

Closed marecar3 closed 4 years ago

marecar3 commented 4 years ago

This PR migrates android support dependencies to AndroidX

It's used by gutenberg-mobile PR

marecar3 commented 4 years ago

Hey, @jtreanor should we increment also this line: https://github.com/wordpress-mobile/AztecEditor-Android/blob/develop/jitpack.yml#L2 to 28 in this PR?

marecar3 commented 4 years ago

Hey, @loremattei @jtreanor @hypest I have fixed all problems with tests which were reproducible on a local machine.

After that, I have experienced problems with failing tests in MixedTextFormattingTests. Problem is that tests are failing randomly and I didn't have any luck to reproduce them locally.

One solution was to add threadSleep in some places and it gave result, but it seems that tests can't be fixed with that approach.

Any ideas what could be the problem here? Thanks!

hypest commented 4 years ago

It seems that even on PRs without code changes (Example: #827), the connected tests fail at the moment with timeouts. So, probably false negatives at this stage. I think it might make sense to revert the commits that add delays... I'll test that out and report back.

EDIT: ran the (unit and connected) tests locally for 07cba880 and they pass. I'm going to revert the "sleep" commits (and 67b3b63) and merge this PR.

jtreanor commented 4 years ago

@hypest @marecar3 You can increase the timeout for connected tests by updating this line to something like 15m. Lets make sure these are running on CI before merging.

jtreanor commented 4 years ago

Hey, @jtreanor should we increment also this line: https://github.com/wordpress-mobile/AztecEditor-Android/blob/develop/jitpack.yml#L2 to 28 in this PR?

@marecar3 yes, that should be updated too.

hypest commented 4 years ago

You can increase the timeout for connected tests by updating this line to something like 15m. Lets make sure these are running on CI before merging.

Thanks for the pointer @jtreanor ! OK, I'll try that but I'd say let's not block on this, especially since it doesn't seem to be specific to this PR anyway.