-
Notifications
You must be signed in to change notification settings - Fork 68
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
UIP-2414: Resolve ddc test failures #122
Merged
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
/// Provides detection and patching of the bug described in <https://github.com/dart-lang/sdk/issues/27647>, | ||
/// in which getters/setters with the identifier `name` don't work for emulated function classes, like [UiProps]. | ||
@JS() | ||
library react.ddc_emulated_function_name_bug; | ||
|
||
import 'package:js/js.dart'; | ||
|
||
/// Create a reduced test case of the issue, using an emulated function pattern that is similar to [UiProps]. | ||
/// | ||
/// We can't use [UiProps] itself, since it uses [isBugPresent], and that would cause a cyclic initialization error. | ||
class _NsmEmulatedFunctionWithNameProperty implements Function { | ||
void call(); | ||
|
||
@override | ||
noSuchMethod(i) {} | ||
|
||
String _name; | ||
|
||
// ignore: unnecessary_getters_setters | ||
String get name => _name; | ||
// ignore: unnecessary_getters_setters | ||
set name(String value) => _name = value; | ||
} | ||
|
||
/// Whether this bug, <https://github.com/dart-lang/sdk/issues/27647>, is present in the current runtime. | ||
/// | ||
/// This performs functional detection of the bug, and will be `true` | ||
/// only in the DDC and only in versions of the DDC where this bug is present. | ||
final bool isBugPresent = (() { | ||
const testValue = 'test value'; | ||
|
||
var testObject = new _NsmEmulatedFunctionWithNameProperty(); | ||
|
||
try { | ||
// In the DDC, this throws: | ||
// TypeError: Cannot assign to read only property 'name' of function 'function call(...args) { | ||
// return call.call.apply(call, args); | ||
// }' | ||
testObject.name = testValue; | ||
} catch(_) { | ||
return true; | ||
} | ||
|
||
try { | ||
// We don't expect accessing this to throw, but just in case... | ||
return testObject.name != testValue; | ||
} catch(_) { | ||
return true; | ||
} | ||
})(); | ||
|
||
|
||
@JS() | ||
@anonymous | ||
class _PropertyDescriptor {} | ||
|
||
@JS('Object.getPrototypeOf') | ||
external dynamic _getPrototypeOf(dynamic object); | ||
|
||
@JS('Object.getOwnPropertyDescriptor') | ||
external _PropertyDescriptor _getOwnPropertyDescriptor(dynamic object, String propertyName); | ||
|
||
@JS('Object.defineProperty') | ||
external void _defineProperty(dynamic object, String propertyName, _PropertyDescriptor descriptor); | ||
|
||
/// Patches the `name` property on the given [object] to have the expected behavior | ||
/// by copying the property descriptor for `name` from the appropriate prototype. | ||
/// | ||
/// This is a noop if `name` is not a property on the given object. | ||
/// | ||
/// __This functionality is unstable, and should not be used when [isBugPresent] is `false`.__ | ||
/// | ||
/// This method also had undefined behavior on non-[UiProps] instances. | ||
void patchName(dynamic object) { | ||
var current = object; | ||
while ((current = _getPrototypeOf(current)) != null) { | ||
var nameDescriptor = _getOwnPropertyDescriptor(current, 'name'); | ||
|
||
if (nameDescriptor != null) { | ||
_defineProperty(object, 'name', nameDescriptor); | ||
return; | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
@Tags(const ["ddcFailure"]) | ||
|
||
import 'dart:js'; | ||
|
||
import 'package:test/test.dart'; | ||
|
@@ -33,7 +35,7 @@ void commonFactoryTests(Function factory) { | |
test('multiple arguments', () { | ||
var instance = factory({}, 'one', 'two'); | ||
expect(getJsChildren(instance), equals(['one', 'two'])); | ||
}); | ||
}, tags: 'ddcFailure'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should have a comment as to why it's disabled in the DDC |
||
|
||
test('a List', () { | ||
var instance = factory({}, ['one', 'two',]); | ||
|
@@ -155,7 +157,7 @@ void _childKeyWarningTests(Function factory) { | |
); | ||
|
||
expect(consoleErrorCalled, isFalse, reason: 'should not have outputted a warning'); | ||
}); | ||
}, tags: 'ddcFailure'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should have a comment as to why it's disabled in the DDC |
||
|
||
test('when rendering custom Dart components', () { | ||
_renderWithUniqueOwnerName(() => | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is this needed? Wouldn't this mark the whole suite as
ddcFailure
?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 I misread the docs about this.. for some reason I thought I needed to declare the tag to be able to use it. I'll take this out.