Create additional integration test cases #143
Conversation
|
Can one of the admins verify this patch? |
|
run tests |
querti
force-pushed
the
add_integration_testcases
branch
from
fa2bd14
to
2c41a65
Compare
|
run tests |
|
@emilyzheng Can you also review pls? |
|
run tests |
1 similar comment
|
run tests |
|
run tests |
| separated_modules = [(module[:module.find(' ')], module[module.find(' ') + 1:]) | ||
| for module in modules] | ||
|
|
||
| for module in separated_modules: |
There was a problem hiding this comment.
Just a thought here.
This serves as a validation of testing data.
I think in future, it should be better to reset testing repositories/units on pulp server before testing to some baseline state.
This can get messed easily and testing then will be unreliable.
This also applies for other parts of code, but this should solved in different task since it might require non trivial effort.
There was a problem hiding this comment.
Currently, the data is only reset before the whole test suite is run. It should be possible to reset it between each test, even though it would make the runtime significantly higher. Another solution might be to prepare exclusive data for each test, so that tests wouldn't interfere with each other. For now, the suite works with the existing data, but additional data would probably be needed in the future if additional test cases are added.
There was a problem hiding this comment.
"mod_name, mod_profile in separated_modules" would be more readable
|
|
||
| modules = query_repo_modules(None, 'ubi', repo_url, full_data=True) | ||
| assert len(modules) == 1, 'Unexpected repo modules found.' | ||
| separated_module = (modules[0][:modules[0].find(' ')], modules[0][modules[0].find(' ') + 1:]) |
There was a problem hiding this comment.
Looks to me a bit crude parsing of yum list module output.
Please consider moving this to some helper method which parse yum output. It's also use on line 270
There was a problem hiding this comment.
I have added a helper function
| assert len(modules) == 1, 'Unexpected repo modules found.' | ||
| separated_module = (modules[0][:modules[0].find(' ')], modules[0][modules[0].find(' ') + 1:]) | ||
| assert separated_module[0] == 'httpd', 'Unknown module in repo.' | ||
| assert '[d]' in separated_module[1], 'Module httpd should have defaults.' |
There was a problem hiding this comment.
Shouldn't this also assert which profile is default? Not just that there some profile was set as default.
There was a problem hiding this comment.
The test should now check for which profile is default
| repo_url = repo_url = get_repo_url(repo['url']) | ||
| break | ||
| repo_rpms = query_repo_rpms(None, 'ubi', repo_url) | ||
| assert expected1[0] in repo_rpms and expected1[1] in repo_rpms, \ |
There was a problem hiding this comment.
It's kind of difficult to review this because I don't how module's artifacts list looks...
|
|
||
| debug_repos = get_repos_from_cs(cfg.content_sets.debuginfo.output, skip_dot_version=True) | ||
| for repo in debug_repos: | ||
| repo_url = repo_url = get_repo_url(repo['url']) |
There was a problem hiding this comment.
What's the intention here?
Also on 575, 549 and other lines.
There was a problem hiding this comment.
Copy-paste gone wrong. I have fixed it
| repo_url = repo_url = get_repo_url(repo['url']) | ||
| break | ||
| repo_rpms = query_repo_rpms(None, 'ubi', repo_url) | ||
| expected1 = ['httpd-2.4.6-88.el7.x86_64.rpm', 'httpd-tools-2.4.6-88.el7.x86_64.rpm', |
There was a problem hiding this comment.
What's the deal with expected1[1] item? Doesn't seem tested in any way.
|
run tests |
| separated_modules = [(module[:module.find(' ')], module[module.find(' ') + 1:]) | ||
| for module in modules] | ||
|
|
||
| for module in separated_modules: |
There was a problem hiding this comment.
"mod_name, mod_profile in separated_modules" would be more readable
| cfg = load_ubiconfig('associate-md.yaml') | ||
| rpm_repos = get_repos_from_cs(cfg.content_sets.rpm.output, skip_dot_version=True) | ||
| for repo in rpm_repos: | ||
| repo_url = repo_url = get_repo_url(repo['url']) |
There was a problem hiding this comment.
I think you have extra repo_url = here, also you break out of the loop after first cycle. Do you need to iterate through the repos at all then?
| modules = query_repo_modules(None, 'ubi', repo_url, full_data=True) | ||
| assert len(modules) == 1, 'Unexpected repo modules found.' | ||
| separated_module = separate_modules(modules)[0] | ||
| assert separated_module[0] == 'httpd', 'Unknown module in repo.' |
There was a problem hiding this comment.
btw, maybe slightly better could be expected:x found:y message
| for rpm in repo_rpms: | ||
| if 'glibc' in rpm: | ||
| found_archs.append(rpm.split('.')[-2]) | ||
| assert all(arch in found_archs for arch in ['i686', 'x86_64']), \ |
There was a problem hiding this comment.
isn't better to test found_arch == set([expected arches])? Then you can omit test bellow
| repo_rpms = query_repo_rpms(None, 'ubi', repo_url) | ||
| assert all('glibc' not in rpm for rpm in repo_rpms), \ | ||
| 'No glibc rpms should be in the output repo.' | ||
|
|
There was a problem hiding this comment.
I wonder if you should also add here to test check input data. That you make it easier to distinguish between failure due missing input data and wrong ubi-pop functionality.
There was a problem hiding this comment.
Jenkins job restores all of the data before running the test suite. Also, Emily told me that I would either need to use the pa_tool or parse the database directly to check the input data. Is setting this up for such an edge case worth it?
querti
force-pushed
the
add_integration_testcases
branch
from
4dccbd7
to
b70dd2e
Compare
|
run tests |
|
run tests |
No description provided.