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
Adding topic inheritance test #237
Conversation
c9fbbda
to
bfd3709
Compare
@adrianomarto I just happened to read this scary pair of sentences in a GitHub email:
So it turns out you've edited it since, but I looked at the test code anyway. It looks to me like the second The other bit is that with it then surprised me that the CI is green. It turns out it skips the tests because of the
In any case, you should be able to use the derived type for a topic without also using the base type as a topic. If that doesn't work, it's a bug, possibly a python-only bug. |
@eboasson, I'd like to point out that you understood my intent correctly. I observed that behaviour while using Cyclonedds v0.10.4 and Cyclonedds-Python v0.10.2. I am still trying to figure out how to reproduce the same behaviour in the CI environment. Yes, I first thought that it was working correctly, but then I noticed that my test was being skipped |
1c71b6c
to
26e3c5c
Compare
I am afraid we have a bug indeed. The test works fine when I publish and subscribe to the Base topic. It also works as expected when I publish and subscribe to the Base and Derived topics. However, the test fails when I only publish and subscribe to the Derived topic. |
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.
Looks good except for one thing: in my understanding of the way all the Python setup happens, it also needs a reference to "pytest-asyncio" in setup.py, because otherwise a pip3 install '.[dev]'
won't install it automatically if it isn't present yet.
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.
LGTM, thank you @adrianomarto !
CI build re-done after merging #239 looks good: https://dev.azure.com/eclipse-cyclonedds/cyclonedds-python/_build/results?buildId=5763&view=results
Tests if derived topics can be published and subscribed.
This test aims to demonstrate the issue #238.