Skip to content
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

Generated GREYAction JS wrapper uses unsupported variable types #228

Closed
rotemmiz opened this issue Aug 14, 2017 · 3 comments · Fixed by #231
Closed

Generated GREYAction JS wrapper uses unsupported variable types #228

rotemmiz opened this issue Aug 14, 2017 · 3 comments · Fixed by #231
Assignees

Comments

@rotemmiz
Copy link
Member

rotemmiz commented Aug 14, 2017

The newly added GREYActions generated code relies on invocation features we don't support natively.

For instance here we use GREYDirection (which is just an ENUM). On the original implementation we us NSInteger to invoke the method.

Description

Here's the invocation action that is sent when using the old method:
{"type":"invoke","params":{"target":{"type":"Invocation","value":{"target":{"type":"EarlGrey","value":"instance"},"method":"detox_selectElementWithMatcher:","args":[{"type":"Invocation","value":{"target":{"type":"Class","value":"GREYMatchers"},"method":"detoxMatcherForScrollChildOfMatcher:","args":[{"type":"Invocation","value":{"target":{"type":"Class","value":"GREYMatchers"},"method":"matcherForAccessibilityID:","args":["ScrollView161"]}}]}}]}},"method":"performAction:","args":[{"type":"Invocation","value":{"target":{"type":"Class","value":"GREYActions"},"method":"actionForScrollInDirection:amount:","args":[{"type":"NSInteger","value":4},{"type":"CGFloat","value":100}]}}]},"messageId":5}

Here's the invocation action that is sent with the new generated code
{"type":"invoke","params":{"target":{"type":"Invocation","value":{"target":{"type":"EarlGrey","value":"instance"},"method":"detox_selectElementWithMatcher:","args":[{"type":"Invocation","value":{"target":{"type":"Class","value":"GREYMatchers"},"method":"detoxMatcherForScrollChildOfMatcher:","args":[{"type":"Invocation","value":{"target":{"type":"Class","value":"GREYMatchers"},"method":"matcherForAccessibilityID:","args":["ScrollView161"]}}]}}]}},"method":"performAction:","args":[{"type":"Invocation","value":{"target":{"type":"Class","value":"GREYActions"},"method":"actionForScrollInDirection:amount:","args":[{"type":"GREYDirection","value":4},{"type":"CGFloat","value":100}]}}]},"messageId":5}

The difference is {"type":"NSInteger","value":4} vs {"type":"GREYDirection","value":4}

Steps to Reproduce

in ios/expect.js, change

this._call = invoke.call(invoke.IOS.Class('GREYActions'), 'actionForScrollInDirection:amount:', invoke.IOS.NSInteger(direction), invoke.IOS.CGFloat(amount));

to

this._call = invoke.callDirectly(GreyActions.actionForScrollInDirectionAmount(direction,amount));

and also, remark the switch on direction.

@rotemmiz
Copy link
Member Author

These are the currently supported types:
EarlGrey, Class, NSString, NSNumber, NSInteger, CGFloat, CGPoint, CGRect and Invocation

@rotemmiz
Copy link
Member Author

We do need to refactor the MethodInvocation part to be more flexible, but for the time being, these are our limitations.

@DanielMSchmidt
Copy link
Contributor

This is actually just a small bug, will provide a fix for it. In the other hands this leads to the discussion on how to fix unsupported types being used.

The current approach is to add sanitizers (e.g. sanitize_GREYDirection) and I think it would be best to go this path until the end and change the invocation scheme to support arbitrary inputs afterwards.

My plan for this would be to

  1. List all supported typed, like @rotemmiz already did
  2. Filter the methods before building the AST (this would be the right place to start)
  3. Add a console.info anywhere here that does the opposite filter and list out which methods / types are currently unsupported.

We could also think about it the other way around and check the generated code for any types other than the ones whitelisted, that way we could be sure to spot typos too.

DanielMSchmidt added a commit that referenced this issue Aug 14, 2017
@wix wix locked and limited conversation to collaborators Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants