-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[e2e] Adding failure details for driver tests #2593
Conversation
packages/e2e/lib/common.dart
Outdated
String get failureDetails => _allTestsPassed ? '' : _failureDetails; | ||
|
||
/// Convert a string pass/fail result to a boolean. | ||
static bool allTestsPassed(String result) => (result == 'pass') |
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.
Does this need to be public?
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.
I am providing as many public methods as possible.
I think you already noticed. This is a breaking change. There are many tests already written using e2e. I am trying to provide methods to developers:
- a method where they can call if they want to keep their string usage ('pass', 'fail')
- a method where they can call for a boolean.
packages/e2e/lib/common.dart
Outdated
} | ||
|
||
/// Method for formating the test failures' details. | ||
String formatFailures(Map<String, String> failureDetails) { |
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.
If failure details are a Map<String, String>
then it can be turned to JSON as is. Why format? Formatting loses information that could be used on the receiving end.
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.
Cause formatting looks better on the output. It adds extra empty lines for example.
For me it increased my productivity since I am able to see things a lot more clear. You don't have to use it in your tests though. This one is not a mandatory method.
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.
For giving more context this is as json:
{"verify text":"══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞═════════════════\nThe following TestFailure object was thrown running a test:\n Expected: <5>\n Actual: <4>\n\nWhen the exception was thrown, this was the stack:\n at Object.wrapException (http://localhost:8080/main.dart.js:3646:17)\n at Object.throwExpression (http://localhost:8080/main.dart.js:3659:15)\n at Object.fail (http://localhost:8080/main.dart.js:16444:16)\n at Object._expect (http://localhost:8080/main.dart.js:16441:9)\n at Object.expect0 (http://localhost:8080/main.dart.js:16417:9)\n at Object.expect (http://localhost:8080/main.dart.js:19769:9)\n at http://localhost:8080/main.dart.js:82327:17\n at _wrapJsFunctionForAsync_closure.$protected (http://localhost:8080/main.dart.js:6615:15)\n at _wrapJsFunctionForAsync_closure.call$2 (http://localhost:8080/main.dart.js:37092:12)\n at StackZoneSpecification__registerBinaryCallback__closure.call$0 (http://localhost:8080/main.dart.js:80369:21)\n\nThe test description was:\n verify text\n═════════════════════════════════════════════════════════════════\n","fail text":"══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞═════════════════\nThe following TestFailure object was thrown running a test:\n Expected: <5>\n Actual: <4>\n\nWhen the exception was thrown, this was the stack:\n at Object.wrapException (http://localhost:8080/main.dart.js:3646:17)\n at Object.throwExpression (http://localhost:8080/main.dart.js:3659:15)\n at Object.fail (http://localhost:8080/main.dart.js:16444:16)\n at Object._expect (http://localhost:8080/main.dart.js:16441:9)\n at Object.expect0 (http://localhost:8080/main.dart.js:16417:9)\n at Object.expect (http://localhost:8080/main.dart.js:19769:9)\n at http://localhost:8080/main.dart.js:82349:17\n at _wrapJsFunctionForAsync_closure.$protected (http://localhost:8080/main.dart.js:6615:15)\n at _wrapJsFunctionForAsync_closure.call$2 (http://localhost:8080/main.dart.js:37092:12)\n at StackZoneSpecification__registerBinaryCallback__closure.call$0 (http://localhost:8080/main.dart.js:80369:21)\n\nThe test description was:\n fail text\n═════════════════════════════════════════════════════════════════\n"}
this is with my format method:
Failure Details:
Failure in method: verify text
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞═════════════════
The following TestFailure object was thrown running a test:
Expected: <5>
Actual: <4>
When the exception was thrown, this was the stack:
at Object.wrapException (http://localhost:8080/main.dart.js:3646:17)
at Object.throwExpression (http://localhost:8080/main.dart.js:3659:15)
at Object.fail (http://localhost:8080/main.dart.js:16453:16)
at Object._expect (http://localhost:8080/main.dart.js:16450:9)
at Object.expect0 (http://localhost:8080/main.dart.js:16426:9)
at Object.expect (http://localhost:8080/main.dart.js:19778:9)
at http://localhost:8080/main.dart.js:82348:17
at _wrapJsFunctionForAsync_closure.$protected (http://localhost:8080/main.dart.js:6615:15)
at _wrapJsFunctionForAsync_closure.call$2 (http://localhost:8080/main.dart.js:37101:12)
at StackZoneSpecification__registerBinaryCallback__closure.call$0 (http://localhost:8080/main.dart.js:80390:21)
The test description was:
verify text
═════════════════════════════════════════════════════════════════
end of failure 1
Failure in method: fail text
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞═════════════════
The following TestFailure object was thrown running a test:
Expected: <5>
Actual: <4>
When the exception was thrown, this was the stack:
at Object.wrapException (http://localhost:8080/main.dart.js:3646:17)
at Object.throwExpression (http://localhost:8080/main.dart.js:3659:15)
at Object.fail (http://localhost:8080/main.dart.js:16453:16)
at Object._expect (http://localhost:8080/main.dart.js:16450:9)
at Object.expect0 (http://localhost:8080/main.dart.js:16426:9)
at Object.expect (http://localhost:8080/main.dart.js:19778:9)
at http://localhost:8080/main.dart.js:82370:17
at _wrapJsFunctionForAsync_closure.$protected (http://localhost:8080/main.dart.js:6615:15)
at _wrapJsFunctionForAsync_closure.call$2 (http://localhost:8080/main.dart.js:37101:12)
at StackZoneSpecification__registerBinaryCallback__closure.call$0 (http://localhost:8080/main.dart.js:80390:21)
The test description was:
fail text
═════════════════════════════════════════════════════════════════
end of failure 2
I think I prefer to use the second one. It is more readable hence increases productivity.
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.
Ok, I stopped using the formatted result. I added a method which developers can use for a formatted output though.
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.
I also added it to our tests:
final common.Response response = common.Response.fromJson(jsonResult);
await driver.close();
if (response.allTestsPassed) {
print('All tests passed.');
exit(0);
} else {
response.formattedFailureDetails;
print('Failure Details:\n${response.formattedFailureDetails}');
exit(1);
}
2dd4da4
to
861d8d2
Compare
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.
Could the failures be returned as a List
? It seems like there's some useful ordering info that will get lost if we do a Map
.
I just realized I missed this one. I'll have a look now :) |
I changed the type to List. |
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.
lgtm with one minor nit
packages/e2e/lib/e2e_driver.dart
Outdated
import 'dart:async'; | ||
import 'dart:io'; | ||
|
||
import 'package:e2e/common.dart' as e2ecommon; |
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.
I would stick to either e2e or common or just don't namespace the import, e2ecommon looks unusual to me.
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.
Thanks for the review!
I'll use e2e then. 'Response' collides with another name. Hence I had to use a namespace.
Merging the PR. Looks like tests are passing. |
* Adding failure details for driver tests * formate fix * addressing reviewer comments * addressing the reviewer comments. part 2 * fix error in response_data * addressing reviewer comments * fixing formatting * addressin reviewer comments. Carrying the repeated driver test code to e2e package * chaning the failure's details type from map to list * format changes * add documentation to public members * formatting * changing namespace name
* Adding failure details for driver tests * formate fix * addressing reviewer comments * addressing the reviewer comments. part 2 * fix error in response_data * addressing reviewer comments * fixing formatting * addressin reviewer comments. Carrying the repeated driver test code to e2e package * chaning the failure's details type from map to list * format changes * add documentation to public members * formatting * changing namespace name
* Adding failure details for driver tests * formate fix * addressing reviewer comments * addressing the reviewer comments. part 2 * fix error in response_data * addressing reviewer comments * fixing formatting * addressin reviewer comments. Carrying the repeated driver test code to e2e package * chaning the failure's details type from map to list * format changes * add documentation to public members * formatting * changing namespace name
Description
Adding failure details for driver tests.
Before the change the failure output was something like:
The updated output:
Related Issues
Fixes: flutter/flutter#51940
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?