-
Notifications
You must be signed in to change notification settings - Fork 34
Install test requirements on startup #115
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
Conversation
It works for me locally. I am not sure why the checks are red. |
I can also make this feature optional. Would it be more favourable to do so? |
6a09e7e
to
3e77dc3
Compare
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 tested multiple scenarios and everything seems to work as far as I'm concerned, except that all the individual test types have to be iterated through no matter whether any of them doesn't exist.
if self.args.test != "all": | ||
self.run_test_command(self.args.plugin, [f"run_{self.args.test}_tests.sh"]) | ||
else: | ||
self.run_test_command(self.args.plugin, self.test_run_scripts) |
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.
There has to be a manual check on which types of tests the given plugin implements. For example, running all types of tests on pulpcore only runs functional tests and fails on performance tests (since there are none to be run), without a chance to start the remaining (unit, lint) tests.
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.
Good catch! 🐶
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 will adjust the scripts to assert the presence of the tests before running them.
function check_test() {
if ![ -d "/path/dir/" ] ...
}
check_test
3e77dc3
to
d5a868f
Compare
@decko, @newswangerd, are we going to merge this PR? It installs all test requirements on boot-up, which could slow the instantiation a bit (not a dealbreaker for me). I find it still useful because we can just run |
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 intentionally left this out of the initialization sequence because it adds a lot of startup time and it's not something that we use very often for galaxy_ng development.
I'd be okay with this as some kind of configuration option or if it could be built into the base image, but this has to be disabled by default.
d5a868f
to
b771618
Compare
Now, the installation is optional. A user can decide whether she wants to install the test requirements or not via the compose file where other settings like "DEV_SOURCE_PATH" are defined. |
2e9ae38
to
928c2c5
Compare
6bfa474
to
6562d0a
Compare
36e1403
to
978b34a
Compare
I really cannot make this PR to pass through the CI checks. This is the error I am experiencing:
This error is raised when I place the "install_test_deps" inside the If there is no way to resolve this issue, I would rather eliminate the requirement for testing the automatic installation in the CI. Everything works locally for me. |
base/utils.sh
Outdated
@@ -36,6 +37,12 @@ install_local_deps() { | |||
done | |||
} | |||
|
|||
install_test_deps() { | |||
if [ "$INSTALL_TESTS" = "True" ]; then | |||
python3 /opt/oci_env/base/container_scripts/install_test_requirements.py |
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.
Instead of using a python script I would just copy-paste the for loop above and change the pip command to install from test_requirements.txt
.
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 did this, but it did not help:
install_test_deps() {
if [ "$INSTALL_TESTS" = "True" ]; then
local src_path_list
IFS=':' read -ra src_path_list <<< "$DEV_SOURCE_PATH"
for item in "${src_path_list[@]}"; do
src_path="/src/${item}"
if [[ -d "$src_path" ]]; then
log_message "Installing requirements for ${item}."
pip3 install -r "${src_path}"/lint_requirements.txt || true
pip3 install -r "${src_path}"/unittest_requirements.txt || true
pip3 install -r "${src_path}"/functest_requirements.txt || true
pip3 install -r "${src_path}"/perftest_requirements.txt || true
fi
done
# python3 /opt/oci_env/base/container_scripts/install_test_requirements.py
fi
}
I am getting the same error. And, I know the error is related to my changes because when I removed the install_test_deps
call from init_container
, it worked.
978b34a
to
d089210
Compare
d089210
to
060bd4f
Compare
9bf2c95
to
2c6fd99
Compare
2c6fd99
to
9f3fca7
Compare
Closing because of the unresolved failures that root in lack of knowledge around the CI. |
closes #56