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

reduce num conforming tests; add missing _conforming test suffix #459

Merged
merged 3 commits into from
Apr 26, 2022

Conversation

diablodale
Copy link
Contributor

Fixes #458

  • adds missing "_conforming" suffix for test cmakelists.txt
  • passes all tests [ctest] 100% tests passed, 0 tests failed out of 26

@themarpe
Copy link
Collaborator

Thanks @diablodale
As discussed a while a go, because of shear number of tests - was thinking to reduce the _conforming to only one or a couple of tests. Maybe /w 14,17 & 20 standards set, like filesystem test, or maybe the device test.

@diablodale
Copy link
Contributor Author

I get it. I was thinking 🤔 about my Device(usb2mode) bug and how test cases didn't catch it because not enough apis were exercised...no get queues...no get/parse frames...etc. I would recommend a conforming test case that parses frames. A simple Script node...

script->setScript(R"(
    import time
    teststr = b'go'
    buf = Buffer(len(teststr))
    buf.setData(teststr)
    while True:
        node.io['out'].send(buf)
        time.sleep(20)
)");

with host code to verify it actually works.

const auto buffer = identDevice.getOutputQueue("out", 2, false)->get<dai::Buffer>(std::chrono::seconds(10), timeout);
const auto msg = buffer->getData();
if (msg == dai_buffer_type{'g', 'o'}) {

I can add this to the device test with most of it copy/paste from my app code.
Use either the existing SysInfo node I added 4938333
or switch to a Script node like above.

@themarpe
Copy link
Collaborator

Sounds good on parsing the frames - I'd go for something else than Buffer - SysInfo sounds good (Buffer doesn't have any metadata to serialize)

@diablodale
Copy link
Contributor Author

pushed commit to strengthen the device test and use sysinfo.

@themarpe
Copy link
Collaborator

Thanks - LGTM
In this case now, should we also reduce the conforming only to this test?

@diablodale
Copy link
Contributor Author

I see repeating errors across CI jobs. I think this is not the PR's code

-- Downloading Depthai device side binaries from server...
-- Downloading depthai and patch
-- commit: 648d4c1ae6dd8c6dc1540721a8960a85de3b761d
-- Status error: 22;"HTTP response code said error"
-- Status error: 22;"HTTP response code said error"
-- Status error: 22;"HTTP response code said error"
-- Status error: 22;"HTTP response code said error"
CMake Error at cmake/DepthaiDownloader.cmake:183 (message):
-- Status error: 22;"HTTP response code said error"
  Aborting.
Call Stack (most recent call first):
  CMakeLists.txt:315 (DepthaiDownload)
-- Couldn't check if depthai-shared codebase matches between device and host
-- Configuring incomplete, errors occurred!

Reduce conforming to only this test?

I think that's reasonable...

  • I assume the CI workflow+tests of luxonis/libnop@4f4943f are working and always run on pushed commits. And that any libnop included via hunter will have passed CI tests. If that is not true, then I recommend that CI be fixed.
  • The gnu/clang preprocessor is cross-platform conforming preprocessor; much of the "conforming" codepath is being comprehensively tested with that.
  • MSVC only enables the conforming preprocessor with explicit flags like /Zc:pre.... (I was surprised it was not the default with c++20.) I suspect there will be a minority of MSVC dev's that enable that flag -- which further reduces the people that might be affected if this PR + libnop CI + gnu/clang testing misses a bug.
  • The bugs I've seen on this topic cause compile failures. It is not hidden/corrupted data on live devices. If/when some bug is not caught by this PR + libnop CI + gnu/clang testing... then that developer can raise the issue to your team for triage.

@themarpe
Copy link
Collaborator

Reran the jobs - we had some issues with server storing artifacts before, but is resolved now.

I assume the CI workflow+tests of luxonis/libnop@4f4943f are working and always run on pushed commits. And that any libnop included via hunter will have passed CI tests. If that is not true, then I recommend that CI be fixed.

Yes, these are run on each commit/PR to master/develop branches - (there is one case right now that I have to fix regarding a clean looking "all pass" CI, but isn't relevant to other tests performed)

Then lets only do it for device test, I agree with the observations.

- drastically reduce number of tests run for
  MSVC conforming preprocessor
  luxonis#459 (comment)
- add option to test harness that indicates
  when a test is run with the MSVC conforming preprocessor
@diablodale diablodale changed the title add missing _conforming suffix to tests cmake reduce num conforming tests; add missing _conforming test suffix Apr 26, 2022
@diablodale
Copy link
Contributor Author

pushed commit to add param on test function to control which tests are run with the MSVC conforming processor
And then updated the tests to use that to limit/reduce the conforming tests to only be run on the device usb speed

@themarpe themarpe merged commit 5a638e7 into luxonis:develop Apr 26, 2022
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.

None yet

2 participants