Add communication logging to Flutter Driver #7485
Conversation
goderbauer
force-pushed
the
logDriverCmds
branch
from
b02af5f
to
2ac1fe0
Compare
| @@ -94,7 +97,9 @@ class FlutterDriver { | |||
| /// Creates a driver that uses a connection provided by the given | |||
| /// [_serviceClient], [_peer] and [_appIsolate]. | |||
| @visibleForTesting | |||
| FlutterDriver.connectedTo(this._serviceClient, this._peer, this._appIsolate); | |||
| FlutterDriver.connectedTo(this._serviceClient, this._peer, this._appIsolate, | |||
| {bool printCommunication: false}) | |||
There was a problem hiding this comment.
nit: spaces around named args
| /// | ||
| /// [printCommunication] determines wheather the command communication between | ||
| /// the test and the app should be printed to stdout. | ||
| static Future<FlutterDriver> connect({String dartVmServiceUrl, bool printCommunication: false}) async { |
| FlutterDriver driver = new FlutterDriver.connectedTo(client, connection.peer, isolate); | ||
| FlutterDriver driver = new FlutterDriver.connectedTo( | ||
| client, connection.peer, isolate, | ||
| printCommunication: printCommunication); |
There was a problem hiding this comment.
nit: if you have a newline after an open parenthesis, you should have a newline before the closing one
| void _logCommunication(String message) { | ||
| if (_printCommunication) | ||
| _log.info(message); | ||
| f.File file = fs.file(p.join(testOutputsDirectory, 'flutter_driver_commands.log')); |
There was a problem hiding this comment.
if you have multiple flutter_driver instances running on the same filesystem, this will result in corruption. Should we attempt to flock() the file while we're writing to it?
There was a problem hiding this comment.
Also this will result in failure if run against a read-only file system or a file system that has no more free disk space.
Maybe we should make saving to disk also optional?
There was a problem hiding this comment.
If you run multiple flutter_driver instances, you should provide them with different testOutputsDirectory. Otherwise, the output in there is meaningless anyways.
Added an option to disable logging to file, but I'll leave it turned on by default.
goderbauer
force-pushed
the
logDriverCmds
branch
from
2ac1fe0
to
88647c1
Compare
| FlutterDriver.connectedTo(this._serviceClient, this._peer, this._appIsolate); | ||
| FlutterDriver.connectedTo(this._serviceClient, this._peer, this._appIsolate, | ||
| { bool printCommunication: false, bool logCommunicationToFile: true }) | ||
| : _printCommunication = printCommunication, _logCommunicationToFile = logCommunicationToFile; |
There was a problem hiding this comment.
How about instead of adding these options we publish a "stream" of log messages and provide utility functions to log them to stdout and files? This way the driver won't have to decide where the log messages go. I think producing log messages and logging them are separate concerns.
One use-case where this could be problematic is in multi-device testing. There you have multiple instances of FlutterDriver, each talking to its own device. If we have them write to the same file, things can get messy.
There was a problem hiding this comment.
As discussed in person: debugging (especially when dealing with random flakes) will be a lot easier if the log is always produced.
To avoid the multi-device testing problem, I added an ID to each driver instance and each instance now logs to flutter_driver_commands_${driverId}.log.
This is handy for debugging a test. Communication is logged to:
* `flutter_driver_commands_{x}.log`, where {x} is an integer, and
* (if requested by user) to stdout
fixes flutter#7473
goderbauer
force-pushed
the
logDriverCmds
branch
from
88647c1
to
3482841
Compare
This is handy for debugging a test. Communication is logged to:
flutter_driver_commands.log(enabled by default)fixes #7473