Add test for remote worker path resolution#97
Conversation
furtib
force-pushed
the
test-plist-resolution-regex
branch
from
d3dfa6b to
761121e
Compare
Szelethus
left a comment
Szelethus
left a comment
There was a problem hiding this comment.
I'm fine with creating/removing the tmp directory with a class level setup/teardown, but by having a global PATH_RESOLUTION, you practically do 90% of the test in those methods. In fact, I'm not even sure the way you are testing there is a need for creating any files. I recommend that you simply test the string replace, which is all that really matters here.
src/codechecker_script.py
Outdated
| """ Remove Bazel leading paths in all files """ | ||
| stage("Fix CodeChecker output:") | ||
| folder = CODECHECKER_FILES | ||
| folder = CODECHECKER_FILES if not pth else pth |
There was a problem hiding this comment.
This is a very intrusive test. You are not testing the entirety of this function, only this part:
data = data_file.read()
for pattern, replace in BAZEL_PATHS.items():
data = re.sub(pattern, replace, data)
Lets just create a separate function that takes some string and returns with a substituted one. One something like that. That way you won't need to add an intrusive parameter just for testing.
There was a problem hiding this comment.
We could just import the list of regexes from here, to not be so intrusive.
furtib
force-pushed
the
test-plist-resolution-regex
branch
from
bea98fe to
7d4745a
Compare
Szelethus
left a comment
Szelethus
left a comment
There was a problem hiding this comment.
Awesome, this is what I wanted to see! I have some nits, but otherwise this is a great PR.
| PATH_RESOLUTION: Dict[str, str] = { | ||
| # {Remote execution absolute path}: {project relative path} | ||
| "/worker/build/5d2c60d87885b089/root/test/unit/legacy/src/lib.cc": "test/unit/legacy/src/lib.cc", | ||
| "/worker/build/a0ed5e04f7c3b444/root/test/unit/legacy/src/ctu.cc": "test/unit/legacy/src/ctu.cc", | ||
| "/worker/build/a0ed5e04f7c3b444/root/test/unit/legacy/src/fail.cc": "test/unit/legacy/src/fail.cc", | ||
| # This resolution is impossible, because "test_inc" => "inc" cannot be resolved | ||
| # "/worker/build/28e82627f5078a2d/root/bazel-out/k8-fastbuild/bin/test/unit/virtual_include/_virtual_includes/test_inc/zeroDiv.h": "test/unit/virtual_include/inc/zeroDiv.h" | ||
| } |
There was a problem hiding this comment.
Why do we need this global variable? We could just pass each of these as an argument.
There was a problem hiding this comment.
I thought it would be easier to test other methods we might add in the future, this way.
furtib
force-pushed
the
test-plist-resolution-regex
branch
from
1a9a825 to
5d312a2
Compare
2 participants
Why:
We want to test our solutions for making remote workers' absolute paths into locally usable relative paths. We should add a test for it.
What:
Adds a new test to run on pre-created remote worker paths.
Addresses:
#65