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

Fix detox issue #618

Merged
merged 14 commits into from
Jan 31, 2024
Merged

Fix detox issue #618

merged 14 commits into from
Jan 31, 2024

Conversation

nazli-stripe
Copy link
Collaborator

@nazli-stripe nazli-stripe commented Jan 30, 2024

Summary

CI has been blocked for a bit due to failing e2e Android tests. We upgraded Detox to ensure our dependencies are officially supported, and discovered an issue with the Permissions API being called too early:
Error: Tried to use permissions API while not attached to an Activity.
Not the prettiest option but adding a 1 second delay before calling the permission checker fixes the issue, and since this is for the example application running on the emulator that seemed good enough to unblock new PRs

Motivation

Unblock CI

Testing

CI

  • I tested this manually
  • I added automated tests

Documentation

Select one:

  • I have added relevant documentation for my changes.
  • This PR does not result in any developer-facing changes.

@nazli-stripe nazli-stripe marked this pull request as ready for review January 31, 2024 06:17
Comment on lines +178 to +180
setTimeout(() => {
handlePermissions();
}, 1000); // delay of 1 second
Copy link
Collaborator

Choose a reason for hiding this comment

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

not new for this PR so no need to block on it but why do we need this in the app layer? Won't the native SDK request and prompt for the permissions when needed? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

The native Android SDK doesn't prompt for permissions itself, it requires the integration to do so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, TIL! thanks 👍

@nazli-stripe nazli-stripe merged commit 2f7fc29 into main Jan 31, 2024
2 checks passed
@nazli-stripe nazli-stripe deleted the fix-detox-issue branch September 6, 2024 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants