Remove currentLocale prepend on iOS #18379
Conversation
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
Is there a test covering this?
|
Tests pending |
|
There are a few places this should be testable, @gaaclarke and @xster have done some work recently to make it even easier from what I understand |
|
If nothing else, soemthing in the scenarios app should cover this. |
|
We also have FlutterEngine unit tests |
| if (!self.exists) { | ||
| return YES; | ||
| } | ||
| usleep(delta * 1000000); |
There was a problem hiding this comment.
Can we avoid this?
Would it help at all if we had EarlyGrey for Flutter instead?
If we need that, we should probably add a TODO here to replace this when EarlGrey is available.
/cc @jmagman who might know a good trick for this
There was a problem hiding this comment.
Something like:
NSPredicate *predicate = [NSPredicate predicateWithFormat:@"exists = YES"];
XCTestExpectation *expectation = [self expectationForPredicate:predicate evaluatedWithObject:textInputSemanticsObject handler:NULL];
[self waitForExpectations:@[expectation] timeout:10.0];There was a problem hiding this comment.
This is probably better, actually:
XCTestExpectation *expectation = [self keyValueObservingExpectationForObject:textInputSemanticsObject keyPath:@"exists" expectedValue:@YES];
[self waitForExpectations:@[expectation] timeout:10.0];(I didn't actually test these).
There was a problem hiding this comment.
It's coming back to me now - that week I got better at Objective C with your guidance .. and how it faded...
There was a problem hiding this comment.
I had to look it up myself, it's been awhile...
There was a problem hiding this comment.
Ahh thanks for the assistance, Trying to learn all the iOS testing frameworks and whatnot to write this haha. This particular code was repurposed from another test, still working on refining/improving it.
| #import <Flutter/Flutter.h> | ||
| #import <XCTest/XCTest.h> | ||
|
|
||
| NS_ASSUME_NONNULL_BEGIN |
There was a problem hiding this comment.
Remove the header, you can move the interface into the test .m file.
| // The locales recieved by dart:ui are exposed onBeginFrame via semantics label. | ||
| // There should only be one locale, as we have removed the locale prepend on iOS. | ||
| XCTAssertTrue([textInputSemanticsObject waitForExistenceWithTimeout:timeout]); | ||
| XCTAssertEqualObjects([textInputSemanticsObject valueForKey:@"hasKeyboardFocus"], @(NO)); |
There was a problem hiding this comment.
I'm guessing textInputSemanticsObject.hasKeyboardFocus isn't an exposed property?
The better typed way to do this when you really have to (ignoring StackOverflow) is to put a category at the top of your implementation file:
@interface XCUIElement (FlutterTests)
@property BOOL hasKeyboardFocus;
@end
...
There was a problem hiding this comment.
And then:
XCTAssertFalse(textInputSemanticsObject.hasKeyboardFocus);There was a problem hiding this comment.
Ok, will keep in mind. I will actually remove this here as it is exercised in other tests.
| - (void)testNoLocalePrepend { | ||
| NSTimeInterval timeout = 10.0; | ||
|
|
||
| XCUIElement* textInputSemanticsObject = |
There was a problem hiding this comment.
I know the engine has variable usage of dot notation, but the "modern" syntax (like circa 2006 modern) of properties would be:
XCUIElement* textInputSemanticsObject = [self.application.textFields matchingIdentifier:@"[en]"].element;
| NSArray<NSString*>* preferredLocales = [NSLocale preferredLanguages]; | ||
| NSMutableArray<NSString*>* data = [[NSMutableArray new] autorelease]; | ||
|
|
||
| // Force prepend the [NSLocale currentLocale] to the front of the list |
There was a problem hiding this comment.
Fun fact: you can change the app's locale and language with launch args -AppleLanguages and -AppleLocale.
https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPInternational/TestingYourInternationalApp/TestingYourInternationalApp.html#//apple_ref/doc/uid/10000171i-CH7-SW2
Alternatively, add AppleLanguages and AppleLocale launch arguments using the scheme editor—for example, add -AppleLanguages "(de)" to specify the German language and -AppleLocale "fr_FR" to specify the France region.
There was a problem hiding this comment.
Thanks, Will be super useful for the next set of changes that I'll be making immediately after this one!
|
Mac iOS Engine LUCI passes on rerun: https://ci.chromium.org/p/flutter/builders/try/Mac%20iOS%20Engine/2826 |
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
LGTM thanks for taking the time to write a good test!
5 participants
Partially reverts #6995
Since we handle null country codes now, this prepend should be safe to remove. [NSLocale currentLocale] is a pre-resolved locale, which can result in the first locale being one that is not preferred by the user. See flutter/flutter#49775