-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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 timing issues in RCTLoggingTests.m #10568
Conversation
@bestander this should fix the logging test failures in tvOS. |
By analyzing the blame information on this pull request, we identified @javache and @nathanajah to be potential reviewers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is great, many thanks, Doug.
Let's wait for CI an merge.
Just a tiny lint nit, could you fix it?
logToConsoleAfterWait: function(str,timeout_ms) { | ||
setTimeout(function() { | ||
console.log(str); | ||
},timeout_ms); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a nit: space is missing after comma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@facebook-github-bot shipit |
@bestander has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: **Motivation** If there are any console log messages that come in on initialization (as will happen right now in tvOS), the RCTLoggingTests can fail intermittently. This change delays the start of the logging test to allow time for initial console messages to come in. Closes facebook/react-native#10568 Differential Revision: D4087974 Pulled By: bestander fbshipit-source-id: 2b0f4a88a74bc6121133317dd909d5bd1f10789b
Summary: **Motivation** If there are any console log messages that come in on initialization (as will happen right now in tvOS), the RCTLoggingTests can fail intermittently. This change delays the start of the logging test to allow time for initial console messages to come in. Closes facebook#10568 Differential Revision: D4087974 Pulled By: bestander fbshipit-source-id: 2b0f4a88a74bc6121133317dd909d5bd1f10789b
Summary: Changelog: [iOS][Fixed] - Fix race condition in RCTLoggingTests integration tests RCTLoggingTests work in the following way: * Put a custom hook on logging (logging function) to intercept log messages * Send several log messages via JS and see whether they hit the hook as expected The problem with this approach was that there may be unexpected log messages, which squeeze inbetween the points of time when the hook was set and when the first message was sent. There was a (now 6 years old!!!) fix to mitigate this problem, which was adding a lead pause of 2s to "make sure" that all the other possible log messages had been sent: facebook#10568 That didn't actually guarantee fixing the problem in general, just partially mitigating it, as the race condition conceptually still remained there. Here I take a different approach, which should guarantee that we skip all the rogue JS messages before we start sending and reading them on our own: * Install the hook * Log a "marker" message * Pump the log until the marker appears - at this point we know that the hook has been definitely installed Differential Revision: D40043442 fbshipit-source-id: 396501cc48eafc9a41f4c5eb4b7d2a5019b0b212
Summary: Pull Request resolved: #34858 Changelog: [iOS][Fixed] - Fix race condition in RCTLoggingTests integration tests RCTLoggingTests work in the following way: * Put a custom hook on logging (logging function) to intercept log messages * Send several log messages via JS and see whether they hit the hook as expected The problem with this approach was that there may be unexpected log messages, which squeeze inbetween the points of time when the hook was set and when the first message was sent. There was a (now 6 years old!!!) fix to mitigate this problem, which was adding a lead pause of 2s to "make sure" that all the other possible log messages had been sent: #10568 That didn't actually guarantee fixing the problem in general, just partially mitigating it, as the race condition conceptually still remained there. Here I take a different approach, which should guarantee that we skip all the rogue JS messages before we start sending and reading them on our own: * Install the hook * Log a "marker" message * Pump the log until the marker appears - at this point we know that the hook has been definitely installed Reviewed By: cipolleschi Differential Revision: D40043442 fbshipit-source-id: b4aa617d27c2dcff26682dd72e47ec19cb0d11ca
Summary: Pull Request resolved: facebook#34858 Changelog: [iOS][Fixed] - Fix race condition in RCTLoggingTests integration tests RCTLoggingTests work in the following way: * Put a custom hook on logging (logging function) to intercept log messages * Send several log messages via JS and see whether they hit the hook as expected The problem with this approach was that there may be unexpected log messages, which squeeze inbetween the points of time when the hook was set and when the first message was sent. There was a (now 6 years old!!!) fix to mitigate this problem, which was adding a lead pause of 2s to "make sure" that all the other possible log messages had been sent: facebook#10568 That didn't actually guarantee fixing the problem in general, just partially mitigating it, as the race condition conceptually still remained there. Here I take a different approach, which should guarantee that we skip all the rogue JS messages before we start sending and reading them on our own: * Install the hook * Log a "marker" message * Pump the log until the marker appears - at this point we know that the hook has been definitely installed Reviewed By: cipolleschi Differential Revision: D40043442 fbshipit-source-id: b4aa617d27c2dcff26682dd72e47ec19cb0d11ca
Motivation
If there are any console log messages that come in on initialization (as will happen right now in tvOS), the RCTLoggingTests can fail intermittently. This change delays the start of the logging test to allow time for initial console messages to come in.