test: remove symlinks from runtime_impl_test test_data. #626
Conversation
Bazel doesn't like symlinks in test data deps (envoyproxy#415). This patch fixes this by removing the symlinks from the test inputs, copying the test inputs to the writable temporary directory and then creating the symlinks during test execution. Using ugly system(3) instead of a shell script wrapper as this is a unit test and we shouldn't need to wrap it.
| // symlinks which can only exist in the test temporary directory to play | ||
| // nice with Bazel, so we create them on-the-fly. Could do this cleanly with | ||
| // std::filesystem if/when we have C++17. | ||
| ASSERT_EQ(0, ::system(("cp -rf --parents test/common/runtime/test_data " + |
There was a problem hiding this comment.
I want to just clarify something for my understanding: If I read the code correctly, there is nothing that requires that the test dirs get cleaned between runs, right? Meaning, in bazel they might, but on my local machine they won't potentially (likely). So tests need to assume that there are previous artifacts? Is that the way to go? Or should we somehow require total cleanup between runs?
There was a problem hiding this comment.
Yeah, it's a precondition that $TEST_TMPDIR is empty, Bazel will provide this. I've updated the test script to pick a fresh tmp dir each time by default to ensure this is true in the no touch case. If someone feeds in explicit $TEST_TMPDIR, the documentation says they should clean it.
I rather do it this way than do an explicit rm -rf, which can get very dangerous when fat fingering the $TEST_TMPDIRvalue.
test/run_envoy_tests.sh
Outdated
| # It is a precondition that both $TEST_TMPDIR and $TEST_SRCDIR are empty. | ||
| if [ -z "$TEST_TMPDIR" ] || [ -z "$TEST_SRCDIR" ] | ||
| then | ||
| TEST_BASE=$(mktemp -d /tmp/envoy_test.XXXXXXXX) |
There was a problem hiding this comment.
Hmm. Actually, I think this is going to cause problems related to why I need #627. Basically. we have Lyft private tests that have configuration files that I need to copy into the runfiles directory before this script runs. If TEST_TMPDIR/TEST_SRCDIR is not defined, which it won't be until we finish bazel migration, I'm going to get 2 different directories each time I do the setup.
Not sure the best way of dealing with this? Is it really horrible to just rm -fr in the case where we are using the default directories?
There was a problem hiding this comment.
I guess in our private test runs I could just figure out a way of injecting env vars via cmake and maybe cleaning the directories.
There was a problem hiding this comment.
I switched to rm -rf, but only in the default case. I didn't like having a fixed default test directory in /tmp, since this plays havoc on multi-user systems or when running multiple test jobs simultaneously. It's a moot point though - when the Bazel works is done, all tests will run under Bazel and run_envoy_tests.sh will be basically bazel test //test/..., so if rm -rf makes it easier for you folks, sure.
|
Looks good, do you mind just merging my change into this one. |
|
I realize even the rm -fr is going to cause issues, since I need to copy stuff into these directories before the script runs. I think this is fine for now. I will just deal with this in our build wrapper until we convert our private build to bazel. |
Description: this PR updates Envoy Mobile's Envoy ref past the point where Envoy internally uses v3 config. I took the chance to update Envoy Mobile's config and delete deprecated fields. Note that SAN verification changes, and thus it was removed here. I opened an issue tracking #630. Further note that this ref update significantly increased the binary size. It is a priority for the team to investigate and trim the binary ahead of the v0.3 release (noted in #447) Risk Level: med boost to v3 config and deleted deprecated fields. Testing: passing liveliness in CI and locally. Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>
Description: this PR updates Envoy Mobile's Envoy ref past the point where Envoy internally uses v3 config. I took the chance to update Envoy Mobile's config and delete deprecated fields. Note that SAN verification changes, and thus it was removed here. I opened an issue tracking #630. Further note that this ref update significantly increased the binary size. It is a priority for the team to investigate and trim the binary ahead of the v0.3 release (noted in #447) Risk Level: med boost to v3 config and deleted deprecated fields. Testing: passing liveliness in CI and locally. Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>
Bazel doesn't like symlinks in test data deps (#415). This patch fixes this by removing the symlinks
from the test inputs, copying the test inputs to the writable temporary directory and then creating
the symlinks during test execution.
Using ugly system(3) instead of a shell script wrapper as this is a unit test and we shouldn't need
to wrap it.