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 test-dds-adapter-depth test #13605

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

maloel
Copy link
Collaborator

@maloel maloel commented Dec 12, 2024

Adds basic streaming test for rs-dds-adapter.

Moved the librs library from dds-specific code to rspy, so other tests can use it.

Tracked on [LRS-1217]

@maloel maloel added ci CI-related: does not change library behavior dds labels Dec 12, 2024
@maloel maloel requested a review from OhadMeir December 12, 2024 17:04
@OhadMeir
Copy link
Contributor

Moved the librs library from dds-specific code to rspy, so other tests can use it.

I am not sure librs is the appropriate name for this in rspy scope. Was named librs because it was a helper to test librealsense DDS capabilities (also testing realdds in same tests folder). Maybe incorporate it into devices.py?
What do you think?

Copy link
Contributor

@OhadMeir OhadMeir left a comment

Choose a reason for hiding this comment

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

Thought about it, we have a live/tools unit test folder. I think the adapter test should be there. Maybe under an internal dds folder

@maloel
Copy link
Collaborator Author

maloel commented Dec 15, 2024

Moved the librs library from dds-specific code to rspy, so other tests can use it.

I am not sure librs is the appropriate name for this in rspy scope. Was named librs because it was a helper to test librealsense DDS capabilities (also testing realdds in same tests folder). Maybe incorporate it into devices.py? What do you think?

It's librealsense, with everything that it has, plus some additions for unit-testing (but not for dds). Has nothing to do with dds.

@maloel
Copy link
Collaborator Author

maloel commented Dec 15, 2024

Thought about it, we have a live/tools unit test folder. I think the adapter test should be there. Maybe under an internal dds folder

I don't agree: everything for dds should be under dds. The 'live' folder is not always right, and using it to group things does not always make sense. That's why we have --live and --not-live as switches for run-unit-tests. It was a good grouping at the time, but it's no longer needed.

If you insist, I'll change.

@OhadMeir
Copy link
Contributor

Thought about it, we have a live/tools unit test folder. I think the adapter test should be there. Maybe under an internal dds folder

I don't agree: everything for dds should be under dds. The 'live' folder is not always right, and using it to group things does not always make sense. That's why we have --live and --not-live as switches for run-unit-tests. It was a good grouping at the time, but it's no longer needed.

If you insist, I'll change.

This test should fail if no USB camera is attached. How is it marked to run only with live cameras now?

@maloel
Copy link
Collaborator Author

maloel commented Dec 15, 2024

Thought about it, we have a live/tools unit test folder. I think the adapter test should be there. Maybe under an internal dds folder

I don't agree: everything for dds should be under dds. The 'live' folder is not always right, and using it to group things does not always make sense. That's why we have --live and --not-live as switches for run-unit-tests. It was a good grouping at the time, but it's no longer needed.
If you insist, I'll change.

This test should fail if no USB camera is attached. How is it marked to run only with live cameras now?

--live does not check the tag or folder - it checks whether it has a test:device directive in it.

@maloel
Copy link
Collaborator Author

maloel commented Dec 16, 2024

--live does not check the tag or folder - it checks whether it has a test:device directive in it.

Not sure you saw this... I got no email...

@OhadMeir
Copy link
Contributor

OK if --live is looking for device directive, but I don't see that your test is running. It is skipped both for GHA and LibCI.

@maloel
Copy link
Collaborator Author

maloel commented Dec 18, 2024

I don't see that your test is running

Good catch.
The reason is that this test is live and DDS at the same time, and we have no setup that runs both together...
We have to make LibCI compile DDS (possibly only on nightly?) @Nir-Az any preference?

@Nir-Az
Copy link
Collaborator

Nir-Az commented Dec 18, 2024

I don't see that your test is running

Good catch. The reason is that this test is live and DDS at the same time, and we have no setup that runs both together... We have to make LibCI compile DDS (possibly only on nightly?) @Nir-Az any preference?

Did a test run, build time increase from 36 [m] to 45[m]
For nightly only it's OK

1 thing I am not sure from the code is if dds on will run only non live test or both live and not live?

@maloel
Copy link
Collaborator Author

maloel commented Dec 18, 2024

1 thing I am not sure from the code is if dds on will run only non live test or both live and not live?

I will make it so that, on nightly only, LibCI will compile DDS. This will be a configuration in Jenkins.
This will also cause the nightly to run all tests -- not just live -- i.e., including DDS -- and this test would then run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci CI-related: does not change library behavior dds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants