search

Igalia / wolvic

A fast and secure browser for standalone virtual-reality and augmented-reality headsets.
https://wolvic.org
Mozilla Public License 2.0
813 stars 105 forks source link

Fix all tests in EnvironmentsTest #1358

Closed HoussemNasri closed 5 months ago

HoussemNasri commented 7 months ago

Summary

The glean-native-forUnitTests and glean-gradle-plugin versions were updated to 55.0.0 to address an issue where running tests resulted in a java.lang.UnsatisfiedLinkError: Error looking up function 'ffi_glean_uniffi_contract_version' exception. Following this, I conducted adjustments to the JSON and unit tests through trial and error until they all passed successfully.

I also went through the deleted changes in this commit https://github.com/Igalia/wolvic/commit/0a9ca17590db25da22283c618c50c3a1be0a125a which I think what made the tests fail to understand the expected behavior of the tests.

The choice of version 55.0.0 was based on its compatibility with Mozilla android components v121. For context: https://github.com/mozilla-mobile/firefox-android/blob/979fbe8d7fe04a9b09fe657bb787fda6f4d5ab42/android-components/plugins/dependencies/src/main/java/DependenciesPlugin.kt#L46

image

HoussemNasri commented 7 months ago

WRT the glean version change, it's weird that v56 is not compatible while v55 is, is it because some removed symbol?

Hmm, I really don't know much about the why, it seems they broke backward compatibility between v55 and v56. I wonder if there is a better way to access the Versions.mozilla_glean variable in DependenciesPlugin.kt to guarantee compatibility with glean in Android components.

Apart from that, I think it'd be nice to add a test phase to the CI, if tests are not automatically run they are not very useful.

Yeah, of course, I'll give it a try.

HoussemNasri commented 6 months ago

Hi @svillar, I added a new workflow to run unit tests. It runs all unit tests in the app module using the debug and then the release build . Also, I made another change: I separated the "deployment" step, which uploads the built APK, into its own job. This way, if the tests don't pass, the upload won't happen.

HoussemNasri commented 6 months ago

I'm still getting this error ERROR: /home/runner/work/wolvic/wolvic/app/build/intermediates/merged_java_res/noapiArm64GeckoGenericRelease/base.jar: R8: com.android.tools.r8.ResourceException: com.android.tools.r8.internal.Te: I/O exception while reading. I thought it was because the branch is outdated, but even rebasing on top of the latest commit in upstream main didn't help. Any ideas how to fix it?

svillar commented 6 months ago

@HoussemNasri OK I don't want you to get blocked on this, and the fixes are still good, so we can figure out the CI later (as it's difficult for you as you don't have full access to CI as we do). Mind removing the changes related to CI? I'll approve and merge after that.

svillar commented 6 months ago

I'm still getting this error ERROR: /home/runner/work/wolvic/wolvic/app/build/intermediates/merged_java_res/noapiArm64GeckoGenericRelease/base.jar: R8: com.android.tools.r8.ResourceException: com.android.tools.r8.internal.Te: I/O exception while reading. I thought it was because the branch is outdated, but even rebasing on top of the latest commit in upstream main didn't help. Any ideas how to fix it?

Ah this is happening without the CI, I thought it was caused by the CI patch. We need to fix it then

HoussemNasri commented 6 months ago

Could it be because the source branch is on my fork, not on upstream? I saw no similar errors in the pull requests that you and the other maintainers opened.

svillar commented 6 months ago

Ah and BTW let's re-add the patch to run tests in CI since the R8 error is not related to that.

HoussemNasri commented 6 months ago

Ah and BTW let's re-add the patch to run tests in CI since the R8 error is not related to that.

Hmmm, I reset hard and force-pushed so it's gone, but it would have needed more work anyway, better add it in another PR.

svillar commented 5 months ago

Superseded by #1441