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

Add unit tests for GSFFIInvocation #466

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

qmfrederik
Copy link
Contributor

Selector forwarding from one object to another seems to be broken when using libobjc2 on Windows (see also: gnustep/plugins-themes-WinUXTheme#7 (comment)).

This PR contains two tests which both pass on Linux, pass on Windows when using the GCC runtime, and fail on Windows when using libobjc2 to demonstrate the problem (and verify a fix).

NSString *fakeUpperCaseString = [fakeString uppercaseString];

PASS_EQUAL(upperCaseString, fakeUpperCaseString, "uppercaseString selector is forwarded from the fake string to the actual NSString object");
NSLog(@"Upper case string: %@, fake upper case string: %@", upperCaseString, fakeUpperCaseString);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should move this line before the test line, otherwise you never see the output for test failures and only there it is relevant.

@fredkiefer
Copy link
Member

fredkiefer commented Nov 17, 2024

Thank you for this test case, this seems very interesting. What I don't understand is, why only one of the tests seems to fail. Shouldn't both behave the same? Sorry, my bad, they both fail, so having just one test case would be enough.

// Forward any invocation to the original item if it supports it...
if ([_originalItem respondsToSelector:selector])
[invocation invokeWithTarget:_originalItem];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the else case you should call the super implementation, to make sure we end up in -doesNotRecognizeSelector:. This won't change the problem, just document the correct way to implement this method.

@fredkiefer
Copy link
Member

@davidchisnall could you please have a look at this issue? The most likely cause seems to be a difference in the bevahiour of libobjc2 on Windows.

@qmfrederik
Copy link
Contributor Author

Thanks @fredkiefer , I made the changes you requested.

You are correct: both tests fail. We can probably keep the GSFakeNSString test and do away with the GSFakeNSMenuItem test. There's a subtle difference in both tests -- in the GSFakeNSString case, selectors are forwarded to a type which is defined in another module; in the GSFakeNSMenuItem both types are defined in the same module. I don't think it matters, though, so one test is probably enough.

@qmfrederik
Copy link
Contributor Author

For completeness: If, in GSFFInvocation.m, in the load method, I comment out the objc_proxy_lookup = gs_objc_proxy_lookup; line, everything works as expected.

@fredkiefer
Copy link
Member

You most likely also get a working result if you don't implement the method -forwardingTargetForSelector:, which is a speed up for libobjc2.

@qmfrederik
Copy link
Contributor Author

You most likely also get a working result if you don't implement the method -forwardingTargetForSelector:, which is a speed up for libobjc2.

That's correct; I split the test in two -- one which implements forwardingTargetForSelector and another which implements forwardInvocation and methodSignatureForSelector.

Copy link
Contributor

@rfm rfm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look good to me.

@qmfrederik
Copy link
Contributor Author

This has been fixed in libobjc2 (gnustep/libobjc2#313) and the fix is being backported to MSYS (msys2/MINGW-packages#22538). I suggest we wait for MSYS to get the libobjc2 fix, after which CI should be green(er) again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants