#139 WIP: adding tests to be run with playbook
Closed 5 years ago by jheger. Opened 6 years ago by jheger.
jheger/standard-test-roles feature/test-filtering  into  master

@@ -48,11 +48,30 @@ 

      - "/usr/share/restraint/*"

      - "rpm.py"

  

+ # Using different variable to workaround bug https://github.com/ansible/ansible/issues/22025

+ - set_fact:

+     test_list: "{{ tests }}"

+ 

+ - name: Generate test list

+   command: "{{ tests_generator_cmd }}"

+   register: cmd_output

+   when:

+     - tests_generator_cmd is defined

+     - tests_generator_cmd != ""

+ 

+ - set_fact:

+     test_list: "{{ test_list + cmd_output.stdout_lines }}"

+   when:

+     - tests_generator_cmd is defined

+     - tests_generator_cmd != ""

+ 

  - name: Copy tests to test environment

    synchronize:

-     src: "{{ playbook_dir }}/"

+     src: "{{ item }}"

      dest: "{{ tenv_workdir }}"

      ssh_args: "-o UserKnownHostsFile=/dev/null"

+   with_items:

+   - "{{ test_list }}"

  

  - name: Fix up beakerlib at tests environment

    shell: >
@@ -93,7 +112,7 @@ 

          echo "FAIL test {{ item }} does not appear to be a file or directory"

        fi

      with_items:

-     - "{{ tests }}"

+     - "{{ test_list }}"

  

    always:

    - name: Make the master tests summary log artifact
@@ -111,7 +130,7 @@ 

          echo "UNKNOWN {{ item }}" >> {{ remote_artifacts }}/test.log

        fi

      with_items:

-     - "{{ tests }}"

+     - "{{ test_list }}"

  

    - include_role:

        name: str-common

JIRA issue: https://projects.engineering.redhat.com/browse/RHELPLAN-3428

This commit adds new variable to standard-role-beakerlib tests_generator_cmd which should contain bash command producing absolute paths to tests (one path per line) you wish to add to list of tests being run with the playbook.

Usage:
tests_generator_cmd can be either written into playbook as variable:

---
# Tests that run in classic context
- hosts: localhost
  roles:
  - role: standard-test-beakerlib
    tags:
    - classic
    tests:
    - func
    - login-shell
    - smoke
    - substitution-in-bash
    required_packages:
    - expect            # login-shell requires expect command
    - findutils         # beakerlib needs find command
    - which             # smoke requires which command
    tests_generator_cmd: 'fmf --brief --key test --format "{}/{}\n" --value "root" --value "data.get(''path'') or name"'

Pull request also changes task which copies tests to working directory (name: Copy tests to test environment), as I found the original way was redundant, I would appreciate feedback on this as well the whole pull request.

Task/variable names are subject to change.

@jheger first of all I would like to see the name of the option to be different and reflect this is extending the tests list. Some suggestions from me:

tests_command: 'CMD'
tests_generetor_command: 'CMD'

please use the synchronize module, the copy is a lot slower :(

this is required for the synchronize module

Hmm, we are now copying only a subset of tests and excluding tests.yml, not sure if that is expected .. needs testing

@jheger, hmm so you careate tests_list but later on use only tests variable to run them? That seems odd ...
https://pagure.io/fork/jheger/standard-test-roles/blob/feature/test-filtering/f/roles/standard-test-beakerlib/tasks/main.yml#_114
?

This is a mistake I forgot to change those variables

1 new commit added

  • Forgot to change variables later in the role
6 years ago

This command will be run on 'test-environment'.

  ls -d ".... full path"

Question is: "Does test-environment system have tests at this point?"

@jheger What is the reason to replace synchronize with copy?

@jheger What is the reason to replace synchronize with copy?

No particular reason, I saw it being used earlier in the role so I used it as well. I can change it to synchronize.

2 new commits added

  • changed module of test copying task
  • changed var name generate_cmd -> tests_generator_cmd
6 years ago

@jheger What is the reason to replace synchronize with copy?

No particular reason, I saw it being used earlier in the role so I used it as well. I can change it to synchronize.

Please do this. PR should have logical changes, that are related to the topic of of PR. If you introduce some change than I assume that it is somehow connected to PR.

Hmm, we are now copying only a subset of tests and excluding tests.yml, not sure if that is expected .. needs testing

@mvadkert actually synchronize src: {{ playbook_dir }} dest: {{ tenv_workdir }} is done before beakerlib-role even starts from role str-common, which is included in beakerlib-role.
So I thought it is enough to copy only the tests to the {{ tenv_workdir }}.

pretty please pagure-ci rebuild

pretty please pagure-ci rebuild

Shall we use something shorter then tests_generator_cmd?
Perhaps tests_command would be enough?

We've discussed this with Miro a bit more and we agreed that the complex syntax using --format and --value is not something that a regular user would be able to quickly understand and easily remember and use. What about using the last example from the wiki?

https://fedoraproject.org/wiki/CI/Metadata/Examples#Integration

We could call the variables fmf_directories or fmf_trees for the list of directories to be inspected and fmf_filters for the list of filters to be applied. This could be a simple step forward without introducing too much complexity. What do you think?

Here's summary from yesterday's discussion with @jheger and @mvadkert: There should be a single filter variable which could used in the following two ways:

For each fetched repository under repositories variable:

repositories:
  - repo: "https://src.fedoraproject.org/tests/selinux.git"
    dest: "selinux"
    filter: "tags:Tier1"

Or a general filter for tests stored directly in dist-git:

  - role: standard-test-beakerlib
    tags:
    - classic
    filter: "tags:Tier1"
    ...

For now filter should support only a single value. In future we can consider supporting list of filters as well if needed.

Support for simple filtering implemented here:
https://pagure.io/standard-test-roles/pull-request/240
We can probably close this pull request now.

Pull-Request has been closed by jheger

5 years ago