-
Notifications
You must be signed in to change notification settings - Fork 327
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
Run integration tests connected to a chrome
device
#5917
Conversation
'debugger_panel_test.dart', | ||
'app_test.dart', | ||
// TODO(https://github.com/flutter/devtools/issues/5874): Enable once supported on web. | ||
// 'eval_and_browse_test.dart', |
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.
web
devicechrome
device
packages/devtools_app/integration_test/test_infra/run/_test_app_driver.dart
Outdated
Show resolved
Hide resolved
assert( | ||
target != null, | ||
'Please specify a test target (e.g. ${testTargetArg}path/to/test.dart', | ||
); |
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.
why are we allowing target to be null now?
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.
We need to parse the args to see if the testAppDevice
arg is provided, but the parsing itself was asserting that the testTarget
wasn't null. Therefore if we were trying to run all web tests (and therefore not specifying a test target), it would fail with the assertion error.
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 think what we want to do instead is leave this assertion here, and move the fix for this issue upstream to run_tests.dart
We can create the TestRunnerArgs from:
[
...testRunnerArgs,
'${TestRunnerArgs.testTargetArg}$testTarget',
],
which is what we pass into the _runTest
method in run_tests.dart
. Instead of passing in a list of strings to that method, let's just pass in these args that you can parse at the top of the main method before you create the testAppDevice
var. Then this assertion won't be hit, but we can still have the guarantee here that this code is not being called with a null test target
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.
Not 100% sure the change I've made is what you asked for, but I added the assertion back and moved the check for whether the test supports the test app device from main
to _runTest
(so that we only do the check once we have a test target). LMK what you think!
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.
Added a couple suggestions below that would need to be paired with this:
if (testTargetProvided) {
final args = TestRunnerArgs(testRunnerArgs);
// TODO(kenz): add support for specifying a directory as the target instead
// of a single file.
await _runTest(args);
} else {
// Run all tests since a target test was not provided.
final testDirectory = Directory(_testDirectory);
final testFiles = testDirectory
.listSync(recursive: true)
.where((testFile) => testFile.path.endsWith(_testSuffix));
for (final testFile in testFiles) {
final testTarget = testFile.path;
final args = TestRunnerArgs([
...testRunnerArgs,
'${TestRunnerArgs.testTargetArg}$testTarget',
]);
await _runTest(args);
}
}
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.
Oh nice! I think I've applied all the suggestions
packages/devtools_app/integration_test/test_infra/run/run_test.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/integration_test/test_infra/run/run_test.dart
Outdated
Show resolved
Hide resolved
'flutter-tester'; | ||
testAppDevice = TestAppDevice.fromArgName( | ||
argWithTestAppDevice?.substring(testAppDeviceArg.length) ?? | ||
'flutter-tester', |
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.
nit: use TestAppDevice.flutterTester.argName instead of the 'flutter-tester' string
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.
one nit and lgtm
auto label is removed for flutter/devtools, pr: 5917, due to - The status or check suite test_ddc has failed. Please fix the issues identified (or deflake) before re-applying this label. |
Follow up to https://dart-review.googlesource.com/c/sdk/+/307970 Learned from Kenzie this was possible :) flutter/devtools#5917 (comment) Bug: #52636 Change-Id: Ic244765a6258c000c4171ece35273609c7ee5929 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/309831 Commit-Queue: Elliott Brooks <elliottbrooks@google.com> Reviewed-by: Ben Konyi <bkonyi@google.com>
Work towards #5703
Follow up to #5854
chrome
devicechrome
device during CIThe goal is to help catch regressions that are specific to web apps.