facebook
Adding testIDs on LogBox to make it easier to detect warnings/errors in automation
Summary:
I'm writing automated E2E testing for my RN feature. The UI flow of our feature sometimes causes a LogBox warning/error. Right now, it's difficult to detect when this LogBox notification pops up because it doesn't have any unique identifier for automation.
In this PR, I'm adding testID's to the root of the Logbox so it's easily findable by test automation frameworks.
Changelog:
Pick one each for the category and type tags:
[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message
[GENERAL] [ADDED] - Add testID to LogBox notification
For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests -->
Test Plan:
I have a Windows computer, so I couldn't figure out a way to test this (test apps are only for iOS and Android). However, it would be easy to test. We simply render a LogBox notification and validate the testID prop populates correctly.
From https://github.com/react-native-community/discussions-and-proposals/issues/656#issuecomment-1560813138
Hi @samuelfreiberg!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at [email protected]. Thanks!
| Platform | Engine | Arch | Size (bytes) | Diff |
|---|---|---|---|---|
| android | hermes | arm64-v8a | 19,429,401 | -16,356 |
| android | hermes | armeabi-v7a | n/a | -- |
| android | hermes | x86 | n/a | -- |
| android | hermes | x86_64 | n/a | -- |
| android | jsc | arm64-v8a | 22,801,922 | -16,369 |
| android | jsc | armeabi-v7a | n/a | -- |
| android | jsc | x86 | n/a | -- |
| android | jsc | x86_64 | n/a | -- |
Base commit: c96c8933745b72148856e62f13553d7a2f5d4f6b Branch: main
One more bit of context specific to testing RN on Windows, since it looks like that’s what’s being targeted, exchanging information from the test app to the out of process test runner was the motivation for https://github.com/microsoft/react-native-windows/tree/main/packages/%40react-native-windows/automation-channel
That didn’t make its way out of the React Native Windows repo though, which makes the current story kinda painful for general OSS apps which need to do anything “gray box” on Windows.
Note that this same suggestion would end up applying to apps using Appium. @kelset
@rickhanlonii has some good reasons why we shouldn’t try to advertise the logbox UI as scrape-able. Our own UI tests are not as “black box” as something like Appium, and I have been told do end up hooking into the console. I’m guessing Detox is also going to be doing something similar to this.
From the proposal, I was expecting this to look more like “add a test ID so we can see if the logbox UI is currently on screen”, instead of using it as a facility to scrape information off of it.
If you’re doing that, I think we’d recommend hooking into one of the proper logging/error reporting APIs. @rickhanlonii might be able to provide more context here and flagged this as the approach to encourage.
I see - so would it be acceptable to place a single testID on the LogBoxButton wrapper?
And what are those proper APIs I should look into? Where can I find info on them?
@NickGerleman I guess I'm trying to find the best, most efficient way for Automation services to find when a Logbox appears and report the Warning/Error in test execution logging
@NickGerleman @kelset @rickhanlonii
The main goal: When we're performing E2E UI automation, we need a quick, efficient way to detect when a Logbox notification has popped up. Additionally, we need a way to:
- Determine if this is a warning or error (different error-handling)
- Obtain the error/warning message to report for debugging purposes
- Close the logbox notification to continue testing
Right now, there is no good solution to this workflow. I'm relying on knowing the UI structure of the Logbox by investigating using Inspect.exe (on windows). Also, there is no way of telling through automation if it's a warning or error.
@samuelfreiberg thanks for submitting the PR and driving this feature.
In my mind, there are three separate requests here, with different solutions for each:
- Detecting if the UI is blocked by LogBox
- Detecting errors/warnings in a running app
- Dismiss LogBox
For the first, I think it would make sense to add a way to detect whether LogBox is open or not so you know if it is blocking the UI in some way. That would allow for a boolean check, but not scraping logbox, because LogBox doesn't display all errors, and scraping the LogBox UI is brittle to us changing the UI in any way in the future. One way to do that would be a single testID on the root view, or we could add an LogBox API like LogBox.onOpen(() => { // fail the test });
For the second, the best way to do this is to tap into to error reporting pipleline with console.error and console.warn. Something like this:
const originalError = console.error;
console.error = function(...args) {
reportFailingTest(...args);
originalError(...args);
}
This won't handle the native errors or native redbox, but neither would LogBox itself.
For the last, I'm curious what the the case is for continuing? The only errors that pop up should be fatal errors that need to be fixed to continue. It's possible to dismiss them, but the app is in a broken state when that happens and it's likely that something else will fail related to the first error, sending you on a chase in the wrong direction. I think the guidance here is to fail on the first error, and report all of the errors/failures captured to the user.
@rickhanlonii Thank you for the input!
For the first bullet point, I agree that adding a testID on the root view would suffice.
For the last bullet point (skipping the 2nd because 1st is the most important), we get log box warnings/errors. In the case of a warning, we'd ideally report the warning, and close it to continue with testing. Warnings are not fatal and can continue.
However, the 2nd two are hackable from our end if we have number one implemented. I'm ok with stripping this PR down to just putting a testID on the root view.
@NickGerleman Please let me know if my latest iteration was more in line with your thinking
This does not look like what Rick was suggesting. I think we want this change to be:
One way to do that would be a single testID on the root view, or we could add an LogBox API like LogBox.onOpen(() => { // fail the test });
And then your own testing may use the other methods he provided.
This does not look like what Rick was suggesting. I think we want this change to be:
One way to do that would be a single testID on the root view, or we could add an LogBox API like LogBox.onOpen(() => { // fail the test });
And then your own testing may use the other methods he provided.
Isn't that what this PR is doing? Adding one testID on the root LogBoxButton wrapper component? Which component were you expecting?
@rickhanlonii Thank you for the input!
For the first bullet point, I agree that adding a testID on the root view would suffice.
For the last bullet point (skipping the 2nd because 1st is the most important), we get log box warnings/errors. In the case of a warning, we'd ideally report the warning, and close it to continue with testing. Warnings are not fatal and can continue.
However, the 2nd two are hackable from our end if we have number one implemented. I'm ok with stripping this PR down to just putting a testID on the root view.
@rickhanlonii @NickGerleman
Re-opening this, as it's become more of an issue lately,
Right now, this PR adds 1 testID on the root LogBoxButton.
Also hoping to get input on the quoted reply: "we get log box warnings/errors. In the case of a warning, we'd ideally report the warning, and close it to continue with testing. Warnings are not fatal and can continue."
@samuelfreiberg I think you might be missing the overall feedback, that Logbox UI isn’t the ideal way to be detecting error state during tests.
@samuelfreiberg I think you might be missing the overall feedback, that Logbox UI isn’t the ideal way to be detecting error state during tests.
All I'm concerned about right now is detecting if the UI is blocked by LogBox. I'm not trying to detect error state. I'm trying to have the ability to detect is a Logbox is open and close it.
Am I misunderstanding what Rick suggested?: "One way to do that would be a single testID on the root view, "
We're having multiple tests fail due to logbox UI popping up. Most of the times, it's a warning that isn't necessarily a cause for concern, and doesn't effect the E2E scenario.
However, it's blocking UI and causing tests to fail. Ideally, we'd be able to query for the Logbox and close it via the button.
How would you suggest we proceed with that if we're not able to have any indication (through automation) that a Logbox has appeared?
Additionally, the test code lives outside of the test app/feature code. When a Logbox happens in-app, I don't think we'd be able to tab into the error reporting pipeline with console.error? So again, we need a way to report the error from the LogBox if so.
@rickhanlonii Any input is appreciated