-
Notifications
You must be signed in to change notification settings - Fork 81
Run the tests in testing-farm #4090
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
base: main
Are you sure you want to change the base?
Conversation
|
/packit test |
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.
Code Review
This pull request adapts the test suite to run in testing-farm by adding a new test runner script, updating Ansible playbooks for environment preparation, and modifying a sanity test. The changes are logical and well-aligned with the goal. I've provided a few suggestions to enhance the new test runner script's robustness and flexibility, and to improve the Ansible playbook for better idempotency and clarity. The typo fix in behave/features/environment.py is also a good catch.
| our_dir=$(readlink -f "$(dirname "$0")") | ||
| tests_dir=$our_dir/../beaker-tests/Sanity/copr-cli-basic-operations |
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.
For better robustness and to avoid issues with relative paths if this script is moved, it's advisable to determine the tests directory path from the repository's top-level directory. Other scripts in this repository, like testing-farm/tests/sanity/test.sh, already use git rev-parse --show-toplevel for this purpose.
| our_dir=$(readlink -f "$(dirname "$0")") | |
| tests_dir=$our_dir/../beaker-tests/Sanity/copr-cli-basic-operations | |
| toplevel=$(git rev-parse --show-toplevel) | |
| tests_dir=$toplevel/beaker-tests/Sanity/copr-cli-basic-operations |
testing-farm/all-on-single-host.sh
Outdated
| if test $# -eq 0; then | ||
| for file in *.sh; do | ||
| run_test "$file" | ||
| done | ||
| fi |
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.
The current implementation only runs tests if no arguments are provided to the script. If arguments are provided, it does nothing. It would be more flexible and intuitive to run all tests by default, but allow running specific tests if they are passed as arguments. This change would make the script's behavior more aligned with common shell script practices.
| if test $# -eq 0; then | |
| for file in *.sh; do | |
| run_test "$file" | |
| done | |
| fi | |
| if [ $# -eq 0 ]; then | |
| set -- *.sh | |
| fi | |
| for file in "$@"; do | |
| run_test "$file" | |
| done |
| - name: test environment hacks, inspired by DockerTestEnv/Dockerfile | ||
| shell: | | ||
| for repo in fedora updates updates-testing; do \ | ||
| dnf -y config-manager setopt $repo.excludepkgs=hello ; done | ||
| echo 'set -g history-limit 40960' > /root/.tmux.conf |
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 task combines two unrelated setup steps. It's better to split them into separate tasks for clarity, maintainability, and idempotency. Using the copy module for creating the .tmux.conf file is idempotent, unlike the echo command in a shell task, which will run on every execution.
- name: Exclude 'hello' package from default repositories
shell: |
for repo in fedora updates updates-testing; do
dnf -y config-manager setopt "${repo}.excludepkgs=hello"
done
- name: Configure tmux history limit
copy:
content: "set -g history-limit 40960"
dest: /root/.tmux.conf
mode: '0644'
Pull Request validationFailed🔴 Failed or pending checks:
🔴 Review - Missing review from a member (2 required) |
|
/packit test |
1 similar comment
|
/packit test |
|
/packit test |
5ea81fd to
68c6549
Compare
|
/packit test |
1 similar comment
|
/packit test |
b5f7214 to
90a23b5
Compare
90a23b5 to
3f88906
Compare
|
/packit test |
|
/packit test |
|
/packit test |
c7e88e0 to
d72fcf2
Compare
d72fcf2 to
f444e8a
Compare
|
/packit test |
All our Copr instances support the needed architectures, even if emulated. The hello package works fine in emulated builds.
pulp is not tested
We are using `which` when detecting if pyp2rpm and gem2rpm is installed.
|
/packit test |
2 similar comments
|
/packit test |
|
/packit test |
|
/packit test |
1 similar comment
|
/packit test |
|
/packit test |
|
/packit test |
|
🎄 surprise - 🎉 https://artifacts.dev.testing-farm.io/a1abbcaf-266f-4383-aec1-1526ae5554f0/ |
No description provided.