-
Notifications
You must be signed in to change notification settings - Fork 2
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
Enables regression testing in bazel. #246
Conversation
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@stonier One quick question as you have experience here. PR in |
I retriggered CI and now it is failing due to another issue not related to this PR:
|
This is fixed |
Going back to this. I assume we have to create a maliput release that ships test_utilities library. Correct? @stonier |
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
BUILD.bazel
Outdated
hdrs = glob(["test/*.h"]), | ||
copts = COPTS, | ||
strip_include_prefix = "test", | ||
visibility = ["//visibility:public"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This library only contains the test/assert_compare.h
header file that is used in serveral test under regression folder.
I tried using visibility:private
but I can't add this target as dep
to a cc_test
statement because there is no visibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to use __subpackages__
, see https://bazel.build/concepts/visibility#visibility-specifications
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Done at ffa0582
Pendings:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL
BUILD.bazel
Outdated
hdrs = glob(["test/*.h"]), | ||
copts = COPTS, | ||
strip_include_prefix = "test", | ||
visibility = ["//visibility:public"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to use __subpackages__
, see https://bazel.build/concepts/visibility#visibility-specifications
# ], | ||
# ) | ||
# Not very bazel style (prefer individual test definitions) | ||
def generate_unit_tests(sources): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest you also allow to pass a list of target dependencies.
That will allow to select in each library the target link dependencies and pair it to what we do in cmake.
I also prefer to have the for loop in each BUILD.bazel file and only pass here the src list with just one file instead of the entire list. It is a matter of composability.
Note you also have the ability to pass in kwargs and default values --> https://bazel.build/rules/macro-tutorial . That could help with target customization. For instance, if we have at some point a test that deserves promotion to size="medium"
we could use default values and override the default "small".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest you also allow to pass a list of target dependencies.
That will allow to select in each library the target link dependencies and pair it to what we do in cmake.
I understand that this is solution is actually better.
However I tried to replicate the current CMake setup, and in the current CMake setup we link to all the targets: https://github.com/maliput/maliput_malidrive/blob/eb547917ac19c1d56efdc5ab09665e49e9571e97/test/CMakeLists.txt
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
If Otherwise, LGTM. |
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
I will proceed and merge it |
Done! |
🎉 New feature
closes #247
Summary
we can enable bazel testing in
maliput_malidrive
Checklist