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

Adds a DBusIgnore annotation #164

Merged
merged 6 commits into from
Mar 31, 2022
Merged

Adds a DBusIgnore annotation #164

merged 6 commits into from
Mar 31, 2022

Conversation

brett-smith
Copy link
Contributor

Adds a DBusIgnore annotation that allows ignoring methods, preventing them being exported. They will neither appear in introspection data, nor can they be called. This is useful when you are exporting a class directly without an interface, but don't want to expose all methods (perhaps they are used internally). It could also be useful on an interface, where both sides are Java and share the same class definition.

This patch will also specifically ignore DBusInterface.getObjectPath(), which is otherwise included when you export a class directly.

…ng them being exported. They will neither appear in introspection data, nor can they be called. This is useful when you are exporting a class directly without an interface, but don't want to expose all methods (perhaps they are used internally). It could also be useful on an interface, where both sides are Java and share the same class definition.

This patch will also specifically ignore `DBusInterface.getObjectPath()`, which is otherwise included when you export a class directly.
@hypfvieh
Copy link
Owner

Would you please provide a unit test case for this feature?

brett-smith and others added 4 commits March 21, 2022 09:30
… infrastructure that I found while attempting to add these new tests.

 * New test added for @DBusIgnore annotation.
 * New test added for enums inside signal constructors
 * Fixed other tests for signals. Some, while there were being sent, no test was being done if they were actually received. While the generic handler had 'expected runs', if the handler is never actually called, you would never know. All the signals tested in `TestAll` now are done in one place, and it always checks all handlers are actually run.
 * Fixes some JPMS meta-data and removes the MRJAR still used in dbus-java-utils.

One final thing that will particularly need review by @ypfvieh when I send in the PR. A few of the tests, namely `TestAll`, `TestDisconnectStuff` and `TestEmptyCollections` I changed to use `withShared(false)`.

This was because when using shared connections, I was finding that only the first test in a single class such as `TestAll` would actually run when run all in one go in Eclipse. All subsequent tests would fail saying that an object had already been exported. If each test was run individually, then they would pass.

This in turn was caused by shared connections. It appears in the `@AfterEach` annotated methods where connections are disconnected, a disconnection does not actually un-export objects. Couple this with the fact that a shared connection will be re-used if it's disconnected. So when the next test starts, the `@BeforeEach` tries to export again *on the same connection*, it is already exported.

This raises a couple of questions.

 * Should `disconnect()` un-export all exported objects?
 * Should connection be re-using connections that are disconnected?

So to work around the issues above, I changed both server and client connections to `withShared(false)`. This then exposed a few other issues with the tests, caused by the fact that both the server and client connections were actually the *same thing*, because they are both using the same address on the session bus. This ends up meaning some tests might pass where otherwise they may have failed, and vice versa.

I have fixed all of the fall-out from this *except* for `testArrays()` where the assertion as to whether the remote `this` is the same as local one now fails, and I can't see why. Switching back to `withShared(true)` for both connections fixes this, but this does not seem to be a fair test or what is intended. I would appreciate some guidance one this one.

To progress from this, I think I need clarification as to what should be happening with the shared connections. For tests to be close to what is happening in real usage, they should not be sharing connections for the local and remote side of the tests.
@brett-smith
Copy link
Contributor Author

Apologies for the delay in getting this done, but it's been a bit of wild ride!

Please see the commit message for 456e00b

@hypfvieh
Copy link
Owner

Thanks for the PR - really a lot of changes.
I have fixed some formatting stuff and will merge now.

@hypfvieh hypfvieh merged commit b9fad06 into hypfvieh:master Mar 31, 2022
@brett-smith
Copy link
Contributor Author

Yeh sorry about the formatting again, I noticed after I had committed some of it wasn't spaces. I've got Eclipse setup to format with spaces now, hopefully will prevent this happening again.

@hypfvieh
Copy link
Owner

hypfvieh commented Mar 31, 2022

@brett-smith
Something is wrong with these changes. After merging, the 'testArrays' method will fail.
mvn clean test-compile surefire:test@jnr-tests -Dtest=TestAll#testArrays

[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.465 s <<< FAILURE! - in org.freedesktop.dbus.test.TestAll [ERROR] org.freedesktop.dbus.test.TestAll.testArrays Time elapsed: 0.289 s <<< FAILURE! org.opentest4j.AssertionFailedError: Didn't get the correct this ==> expected: <org.freedesktop.dbus.test.helper.SampleClass@2be48336> but was: <:1.293:/TestAll:null> at org.freedesktop.dbus.test.TestAll.testArrays(TestAll.java:654)

Can you please take a look at this?

Edit:
I noticed that the log output is different than before. After the patch the introspection-data gets printed to console, before the patch there is no inspection data printed.

@hypfvieh
Copy link
Owner

hypfvieh commented Apr 4, 2022

@brett-smith Any news on this?
If this doesn't get fixed, I will remove the changes introduced with this PR.

@brett-smith
Copy link
Contributor Author

I'm sorry I totally missed this reply. I am just about to use dbus-java in a new project and came to see what's changed and stumbled on this.

I see@DBusIgnore is still in the source tree, and the unit tests are now passing. So I guess it's all now OK anyway.

FWIW, my original commit message did mention testArrays(). It was something to do with the shared connection.

I have fixed all of the fall-out from this *except* for `testArrays()` where the assertion as to whether the remote `this` is the same as local one now fails, and I can't see why. Switching back to `withShared(true)` for both connections fixes this, but this does not seem to be a fair test or what is intended. I would appreciate some guidance one this one.

To progress from this, I think I need clarification as to what should be happening with the shared connections. For tests to be close to what is happening in real usage, they should not be sharing connections for the local and remote side of the tests.

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

Successfully merging this pull request may close these issues.

2 participants