[test-suite] Fix FBNativeAd test suite by adding onError support #8662
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
All strings must be wrapped with Text components to be rendered properly
This lets us specify a longer timeout which comes in handy
…ager This will let us easily scope errors to the specific manager that errored
We will need to support more than one event subscription soon
…possible to load an ad
…ating errors to placement IDs
|
Native Component List for this branch is ready |
esamelson
approved these changes
Jun 4, 2020
esamelson
pushed a commit
that referenced
this pull request
Jun 9, 2020
…thNativeAd (#8662) # Why Fixes #8643. # How The problems that I faced were: - the test timed out, most probably due to `mountAndWaitFor` not being able to finish fetching new ad (debugging natively I saw that `setNativeAd` was being hit after the tests failed), so I moved the `mountAndWaitFor` to inside of the test and added a higher timeout value - this has resulted in "text must be wrapped in an explicit <Text> component" error, which was occurring due to `headline` being `""` which is falsy and makes `headline && <Text>…</Text>` resolve to `""`. I revamped the construct to `? : null` which fixed that issue. - after longer testing it turned out that on Android I'm able to reproduce timeouts even with the higher allowed value (with a clean install) — this got me thinking there must be more to it than just the timeout - so I dived into native code and saw the `todo: handle errors`. It turned out that timeout is actually an error, just not delivered. So I added the `onError` callback for both platforms. Hooray! 🎉 # Test Plan When the test fails, the specific reason is shown as Promise rejection. It failed on my device which has ad tracking limits enabled. It failed on clean installs on Android (restarting fixed the issue).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
Fixes #8643.
How
The problems that I faced were:
mountAndWaitFornot being able to finish fetching new ad (debugging natively I saw thatsetNativeAdwas being hit after the tests failed), so I moved themountAndWaitForto inside of the test and added a higher timeout valueheadlinebeing""which is falsy and makesheadline && <Text>…</Text>resolve to"". I revamped the construct to? : nullwhich fixed that issue.todo: handle errors. It turned out that timeout is actually an error, just not delivered. So I added theonErrorcallback for both platforms. Hooray! 🎉Test Plan
When the test fails, the specific reason is shown as Promise rejection. It failed on my device which has ad tracking limits enabled. It failed on clean installs on Android (restarting fixed the issue).