Acceptance criteria for code changes

[vicvicg]

When running CacheLib tests following the instructions (https://cachelib.org/docs/installation/testing), we get different test pass rates depending on the environment, some test failures seem to be intermittent, and we haven’t seen 100% pass rate. Is there a recommended system set up and subset of tests that we can use as an acceptance criteria for code changes?

Our best result:
docker based on centos8, 3 tests failed:
allocator-test-NvmCacheTests
common-test-TimeTests
shm-test-test_page_size

We can share more details if needed.

[sathyaphoenix]

@vicvicg The shm-test is understandable to fail since it depends on system configuration for SHM. The other two should not be flaky. Can you share more details on the failures for those two. Also looping in @agordon for any additional pointers.

[victoria-mcgrath]

When I re-run allocator-test-NvmCacheTests and common-test-TimeTests manually, they pass. But when all tests are run these two fail consistently and empty *.fail files are generated. There's no core dump at the end. Please find .log files attached to this comment and please let me know if there's anything else you need.
common-test-TimeTests.log
allocator-test-NvmCacheTests.log

We also tried enabling automatic test run via github action using self-hosted runners and it seems that cachebench suite of tests is either taking an unacceptably long time or just hanging. Do you have instructions / recommendations on how to enable per-check-in automated testing?

[sathyaphoenix]

Reg the acceptance criteria for the code, we do expect the unit tests to pass. It might help if you can describe the change you are trying to make to give us some context on why the failures might be happening.

When I re-run allocator-test-NvmCacheTests and common-test-TimeTests manually, they pass. But when all tests are run these two fail consistently and empty *.fail files are generated.

This seems like an issue with the test/build environment more than a discussion. So, let me create an issue to track this on our end. The failure seems to be the following which seems to be a combination of timing related (a stressed container for example) and poorly written test.

/opt/workspace/cachelib/allocator/nvmcache/tests/NvmCacheTests.cpp:385: Failure
Expected: (hdl) != (nullptr), actual: nullptr vs (nullptr)

We also tried enabling automatic test run via github action using self-hosted runners and it seems that cachebench suite of tests is either taking an unacceptably long time or just hanging.

I am confused by this statement. The tests referenced here are unit tests and different from cachebench which is a stress test. Cachebench runs for a longer duration until it exhausts all the operations, so that might be the reason why you think it hangs ?

Do you have instructions / recommendations on how to enable per-check-in automated testing?

When you open a PR and someone from facebook imports it, there will be automatic test runs scheduled on the facebook test/dev environment. Once code lands, and the PR is synced to github, it should trigger the contbuild tests on github for the OSS platforms we currently support.

[vicvicg]

Hi @sathyaphoenix,
At the moment we're trying to setup build and test baseline on unchanged version of CacheLib source tree so that we can keep track of regressions as we are making changes to make sure that the PR acceptance is expediated. I’ll attach a docker file that we use for build/test to the issue that you created (this would be the container with which we see those test failures which I attached the logs for).

We enabled "on:push" action that runs this workflow: https://github.com/facebook/CacheLib/blob/main/.github/workflows/build-cachelib.yml. After we made sure that build step passes, we added testing step at the end and observed the runner hanging on TimeStampTickerTest test from cachebench suite, I tried disabling it but then runner just hung on the next one from the suite:
- name: "run CacheLib tests"
run: |
cd opt/cachelib/tests
make -j
- uses: actions/upload-artifact@v2
if: failure()
with:
name: cachelib-test-logs
path: |
./opt/cachelib/tests/cachelib-test-summary.log
if-no-files-found: warn
retention-days: 1

1. Could you advise on changing the test part of workflow to only include tests that are supposed to be used for per-check-in testing?
2. Could you share environment and build/instructions that we could use to replicated “contbuild tests on github for the OSS platforms we currently support”?

[sathyaphoenix]

@vicvicg The timer test failure seems related to container being slow. It is a poorly written test, which should accomodate for some slack in timing measurement (feel free to send a PR if you are interested in fixing the code). The NvmCaccheTests.ConcurrentFills is a still needs to be debugged.

Could you advise on changing the test part of workflow to only include tests that are supposed to be used for per-check-in testing?

The tests you are dealing with here are the full list that we run internally for code acceptance.

Could you share environment and build/instructions that we could use to replicated “contbuild tests on github for the OSS platforms we currently support”?

I mis-spoke. It appears that our github actions run only runs the build at the moment and not the tests in OSS platform. This is something we are still working on. cc @agordon