-
Notifications
You must be signed in to change notification settings - Fork 6k
iOS platform view mutation XCUITests #11652
iOS platform view mutation XCUITests #11652
Conversation
#import "PlatformViewUITestUtil.h" | ||
#include <sys/sysctl.h> | ||
|
||
const NSInteger kTimeToWaitForPlatformView = 5; |
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.
Let's make this higher. Particularly on a simulator on a bogged down CI machine this might be slow. There's no harm in giving it 30 seconds before we fail the whole test.
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 should also have a comment here explaining that this is in seconds.
XCTFail(@"It took longer than %@ second to find the platform view." | ||
@"There might be issues with the platform view's construction," | ||
@"or with how the scenario is built.", | ||
@(kTimeToWaitForPlatformView)); |
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: can this just be moved to setUp
after launching?
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.
Well, all of these similar ones to either setUp or some utility/super method to avoid the repetition
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.
Asserting in -setUp can have weird test failures, the details of which are slipping my mine at the moment (it says some test failed but doesn't say which?). However refactoring out into a method that's used at the top of each test would work (assuming the bigger subclass refactor I suggest doesn't get done).
XCTAssertTrue([PlatformViewUITestUtil compareImage:golden toOther:screenshot.image]); | ||
} | ||
|
||
@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.
For pretty much all of these - maybe just one method that wrapps all the common behavior and takes in the expected golden file name?
|
||
final PictureRecorder recorder = PictureRecorder(); | ||
final Canvas canvas = Canvas(recorder); | ||
canvas.drawCircle(const Offset(50, 50), 50, Paint()..color = const Color(0xFFABCDEF)); |
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.
Can we do something like this in the base case? It seemed valuable to me to make sure that Flutter drawn UI would still show up with or over a platform view.
final SceneBuilder builder = SceneBuilder(); | ||
|
||
builder.pushOffset(0, 0); | ||
builder.pushClipRRect(RRect.fromLTRBAndCorners(50, 50, 300, 300, topLeft:Radius.circular(15), topRight:Radius.circular(50), bottomLeft:Radius.circular(50))); |
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 - add some trailing commas and split up this long line
|
||
builder.pushOffset(0, 0); | ||
Path path = Path(); | ||
path.moveTo(200, 0); |
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 - add a comment explaining what shape you're expecting to build here.
matrix4[15] = 1.0; | ||
|
||
|
||
// rotate for 1 degree radians |
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'm pretty sure this should just be rotate 1 radian
.
It also might be worht just using Matrix4
here and taking a dependency on vector_math, but probably not a big deal either way.
} | ||
} | ||
} |
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.
new line at EOF
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.
Over all looks good, mostly just nits.
@jmagman might have some good tips about structuring the Unit tests and generally reviewing the Objective C :) |
// found in the LICENSE file. | ||
|
||
#import <XCTest/XCTest.h> | ||
#import "../Scenarios/TextPlatformView.h" |
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 expect to not need a relative path:
#import "TextPlatformView.h"
XCTFail(@"It took longer than %@ second to find the platform view." | ||
@"There might be issues with the platform view's construction," | ||
@"or with how the scenario is built.", | ||
@(kTimeToWaitForPlatformView)); |
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.
Asserting in -setUp can have weird test failures, the details of which are slipping my mine at the moment (it says some test failed but doesn't say which?). However refactoring out into a method that's used at the top of each test would work (assuming the bigger subclass refactor I suggest doesn't get done).
NSBundle* bundle = [NSBundle bundleForClass:[self class]]; | ||
NSString* goldenName = [NSString | ||
stringWithFormat:@"golden_platform_view_cliprect_%@", [PlatformViewUITestUtil platformName]]; | ||
NSString* path = [bundle pathForResource:goldenName ofType:@"png"]; |
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.
Prefer NSURL APIs over NSString path ones. I realized you copied this from @dnfield's code, but I missed it in that code review! Unfortunately there isn't a +[UIImage imageWithData:] API so it adds a local.
NSURL* goldenURL = [bundle URLForResource:goldenName withExtension:@"png"];
NSData* goldenData = [NSData dataWithContentsOfURL:goldenURL]
UIImage* golden = [[UIImage alloc] initWithData:goldenData];
// Clip Rect Tests | ||
@interface PlatformViewMutationClipRectTests : XCTestCase | ||
|
||
@property(nonatomic, strong) XCUIApplication* application; |
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.
Can this file be DRY'd out? It looks like a lot of boilerplate.
Maybe something like this? I wrote this up but didn't try to compile, so check it first.
@interface GoldenPlatformViewTests : XCTestCase
- (instancetype)initWithLaunchArguments:(NSString*)platformViewLaunchArguments goldenName:(NSString*)goldenName NS_DESIGNATED_INITIALIZER;
- (instancetype)init NS_UNAVAILABLE;
@end
@interface GoldenPlatformViewTests ()
@property (copy) NSString* platformViewLaunchArguments;
@property (copy) NSString* goldenName;
@end
@implementation GoldenPlatformViewTests
- (instancetype)initWithLaunchArguments:(NSString*)platformViewLaunchArguments goldenName:(NSString*)goldenName {
self = [super init];
_platformViewLaunchArguments = platformViewLaunchArguments;
_goldenName = goldenName;
return self;
}
- (void)setUp {
[super setUp];
self.continueAfterFailure = NO;
self.application = [[XCUIApplication alloc] init];
self.application.launchArguments = @[ self.platformViewLaunchArguments ];
[self.application launch];
}
// Note: don't prefix with "test" or GoldenPlatformViewTests will run instead of the subclasses.
- (void)checkGolden {
XCUIElement* element = self.application.textViews.firstMatch;
BOOL exists = [element waitForExistenceWithTimeout:kTimeToWaitForPlatformView];
if (!exists) {
...
}
}
@end
@interface PlatformViewMutationClipRRectTests : GoldenPlatformViewTests
- (instancetype)init NS_DESIGNATED_INITIALIZER;
- (instancetype)initWithLaunchArguments:(NSString*)platformViewLaunchArguments goldenName:(NSString*)goldenName NS_UNAVAILABLE;
@end
@implementation PlatformViewMutationClipRectTests
- (instancetype)init {
NSString* goldenName = [NSString
stringWithFormat:@"golden_platform_view_cliprect_%@", [PlatformViewUITestUtil platformName]];
return [super initWithLaunchArguments:@"--platform-view-cliprect" goldenName:goldenName];
}
@end
- (void)testPlatformView {
[self checkGolden];
}
|
||
@interface PlatformViewUITestUtil : NSObject | ||
|
||
+ (NSString*)platformName; |
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.
This should be a class property.
@property (class, copy, readonly) NSString* platformName;
|
||
extern const NSInteger kTimeToWaitForPlatformView; | ||
|
||
@interface PlatformViewUITestUtil : NSObject |
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.
Instead of this utility class, can you make a GoldenImage class with a UIImage property, then implement -isEqual: and -hash?
Then you can initialize it with a name prefix, and it can have a goldenName property that uses the hardware logic, and even figure out the launch args.
If you do that, then my above suggested GoldenPlatformViewTests initializer could become:
- (instancetype)initWithGoldenImage:(GoldenImage*)goldenImage;
Updated the PR! @dnfield @jmagman |
testing/scenario_app/ios/Scenarios/ScenariosUITests/PlatformViewGoldenTestManager.m
Outdated
Show resolved
Hide resolved
// The left side of the rectangle becomes a symmetric curve towards the left. | ||
// The right side of the rectangle becomes a double curve. From top of bottom of the curve, it goes left then right. | ||
// _______ | ||
// | | |
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: Maybe just a reference to the golden image for this, or a link to it on github instead of the ASCII art.
|
||
builder.pushOffset(0, 0); | ||
final Matrix4 matrix4 = Matrix4.identity(); | ||
matrix4.rotateZ(1.0); |
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:
final Matrix4 matrix4 = Matrix4.identity()
..rotateZ(1.0)
..scale(0.5, 0.5, 1.0)
..translate(1000.0, 100.0, 0.0);
matrix4.scale(0.5, 0.5, 1.0); | ||
matrix4.translate(1000.0, 100.0, 0.0); | ||
|
||
final Float64List matrix4_64 = Float64List.fromList(matrix4.storage); |
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.
You don't need the copy here - just do builder.pushTransform(matrix4.storage)
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.
matrix4.storage is a Float32List, builder.pushTransform(matrix4.storage)
throws a compile 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.
Ahh - make sure you import vector_math_64.dart
:)
const int _valueInt32 = 3; | ||
const int _valueFloat64 = 6; | ||
const int _valueString = 7; | ||
const int _valueUint8List = 8; | ||
const int _valueMap = 13; | ||
final Uint8List message = Uint8List.fromList(<int>[ | ||
_valueString, | ||
'create'.length, // this is safe as long as these are all single byte characters. | ||
'create' | ||
.length, // this is safe as long as these are all single byte characters. |
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.
Please move this back up to line 186 - it's ok if the comment goes over 80. Maybe just ditch the comment, or rephrase as // this won't work if we use multibyte characters
testing/scenario_app/pubspec.yaml
Outdated
@@ -8,3 +8,4 @@ dependencies: | |||
path: ../../../out/host_debug_unopt/gen/dart-pkg/sky_engine | |||
sky_services: | |||
path: ../../../out/host_debug_unopt/gen/dart-pkg/sky_services | |||
vector_math: any |
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.
Please use a specific version for vector_math, e.g. ^2.0.8
Looking better! A few comments on the Dart side, I'll defer to @jmagman to finish out the ObjC review but it looks better overall. |
@@ -6,6 +6,7 @@ import 'dart:convert'; | |||
import 'dart:io'; | |||
import 'dart:typed_data'; | |||
import 'dart:ui'; | |||
import 'package:vector_math/vector_math.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.
you want import 'package:vector_math/vector_math_64.dart';
|
||
@interface GoldenImage : NSObject | ||
|
||
@property(readonly, strong, nonatomic) NSString* goldenName; |
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.
copy
// Compare this GoldenImage to `image`. | ||
// | ||
// Return YES if the `image` of this GoldenImage have the same pixels of provided `image`. | ||
- (BOOL)compareGoldenToImage:(nonnull UIImage*)image; |
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.
Remove nonnull, you are in a NS_ASSUME_NONNULL_BEGIN
. You only need to mark nullables.
|
||
@interface GoldenImage () | ||
|
||
@property(readwrite, strong, nonatomic) NSString* goldenName; |
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.
Remove all these read-write properties, you are setting their backing instance variable in the init so the setters aren't called anywhere.
|
||
@property(readwrite, strong, nonatomic) NSString* goldenName; | ||
@property(readwrite, strong, nonatomic) UIImage* image; | ||
@property(strong, nonatomic) NSString* platformName; |
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.
Remove this property, you don't need the backing instance variable since you only use it one time in init to create _goldenName.
You can make it a NS_INLINE function:
NS_INLINE NSString* _platformName() {
NSString* simulatorName =
[[NSProcessInfo processInfo].environment objectForKey:@"SIMULATOR_DEVICE_NAME"];
if (simulatorName) {
return [NSString stringWithFormat:@"%@_simulator", simulatorName];
}
size_t size;
sysctlbyname("hw.model", NULL, &size, NULL, 0);
char* answer = malloc(size);
sysctlbyname("hw.model", answer, &size, NULL, 0);
NSString* results = [NSString stringWithCString:answer encoding:NSUTF8StringEncoding];
free(answer);
return results;
}
return self; | ||
} | ||
|
||
- (BOOL)compareGoldenToImage:(UIImage*)image { |
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.
Override -isEqual
: and -hash
instead.
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.
Override
-isEqual
: and-hash
instead.
Oh actually never mind, that would require you to make a GoldenImage out of the screen shot. This is fine.
|
||
static NSDictionary* _launchArgsMap; | ||
|
||
+ (void)initialize { |
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.
Overriding +initialize
is very frowned upon. You can make the static dictionary a local variable in -initWithIdentifier. Or create the tested golden out of the launch args. Or make it an extern
somewhere to be used by AppDelegate as @dnfield suggests.
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.
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.
Ahh right.
|
||
builder.pushOffset(0, 0); | ||
final Matrix4 matrix4 = Matrix4.identity() | ||
..rotateZ(1.0) |
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 agian: indent these :)
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.
Dart code and overall approach LGTM. Will defer to @jmagman for rest of ObjC review
Updated the objc code. I think it is ready for another review. I ended up adding duplicate maps in AppDelegate and PlatformViewGoldenTestManager. |
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 nits about the static dictionary, fast enumeration, and auditing for generics.
Thank you for making all these changes, it looks good!
}; | ||
}); | ||
BOOL hasGoldenLaunchArg = NO; | ||
for (NSString* key in launchArgsMap.allKeys) { |
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.
Use fast-enumeration for iterating dictionaries instead of allocating a new array and then doing a value lookup later.
__block BOOL hasGoldenLaunchArg = NO;
[launchArgsMap enumerateKeysAndObjectsUsingBlock:^(NSString* argument, NSString* testName, BOOL *stop) {
if ([[[NSProcessInfo processInfo] arguments] containsObject:argument]) {
[self readyContextForPlatformViewTests:testName];
hasGoldenLaunchArg = YES;
*stop = YES;
}
}];
self.window.rootViewController = flutterViewController; | ||
} else { | ||
// The launchArgsMap should match the one in the `PlatformVieGoldenTestManager`. | ||
static NSDictionary* launchArgsMap; |
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.
application:didFinishLaunchingWithOptions:
will only be called once, so you don't need the overhead of the static and dispatch_once.
Also, always use generics.
NSDictionary<NSString*, NSString*>* launchArgsMap;
…lutter#11652)" This reverts commit b73cfda.
git@github.com:flutter/engine.git/compare/834cd15010b2...31e5149 git log 834cd15..31e5149 --no-merges --oneline 2019-09-16 iska.kaushik@gmail.com Revert "Revert "Roll src/third_party/dart a554c8be6b..d0052c1b31 (22 commits)" (#12290)" (flutter/engine#12291) 2019-09-16 iska.kaushik@gmail.com Revert "Add iOS platform view mutation XCUITests to the scenario app (#11652)" (flutter/engine#12292) 2019-09-15 ychris@google.com Add iOS platform view mutation XCUITests to the scenario app (flutter/engine#11652) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jimgraham@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
git@github.com:flutter/engine.git/compare/834cd15010b2...31e5149 git log 834cd15..31e5149 --no-merges --oneline 2019-09-16 iska.kaushik@gmail.com Revert "Revert "Roll src/third_party/dart a554c8be6b..d0052c1b31 (22 commits)" (flutter#12290)" (flutter/engine#12291) 2019-09-16 iska.kaushik@gmail.com Revert "Add iOS platform view mutation XCUITests to the scenario app (flutter#11652)" (flutter/engine#12292) 2019-09-15 ychris@google.com Add iOS platform view mutation XCUITests to the scenario app (flutter/engine#11652) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jimgraham@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
platform_view.dart
so it is easier to add a new platform related scenarioPlatformViewUITests.m
so it is easier to add new platform view related UITests@dnfield let me know if you prefer smaller patches.
All the platform view mutation updates will be unblocked after landing this.