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
tests: fmf/tmt combination for tests in upstrean and Fedora #1192
Conversation
Build succeeded.
|
Build succeeded.
|
Build succeeded.
|
Build failed.
|
Build succeeded.
|
Build succeeded.
|
Build succeeded.
|
Build failed.
|
Build failed.
|
Build failed.
|
Build failed.
|
Build failed.
|
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.
Thank you for doing the translation!
Just one ask to give the configure_git function some more care.
@@ -187,3 +188,22 @@ def copr_client_mock(get_list_return=None): | |||
|
|||
copr_mock = flexmock(mock_chroot_proxy=flexmock(get_list=lambda: get_list_return)) | |||
return copr_mock | |||
|
|||
|
|||
def configure_git(): |
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'd prefer this being a fixture set for the whole test session: https://docs.pytest.org/en/stable/fixture.html#autouse-fixtures-fixtures-you-don-t-have-to-request
tests/conftest.py
Outdated
output = subprocess.check_output("git config -l --global", shell=True) | ||
except subprocess.CalledProcessError: | ||
output = "" | ||
if "user.name" in output if isinstance(output, str) else output.decode(): | ||
return | ||
for item in CMDS: | ||
subprocess.call(item, shell=True) |
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.
please drop shell=True since it's not needed
Build succeeded.
|
plans/session-recording.fmf
Outdated
- how: shell | ||
name: requre_copr | ||
order: 20 | ||
script: 'dnf -y copr enable jscotka/requre' |
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.
Note: Once the new tmt is released and deployed in the pipeline the following should be enough:
prepare:
how: install
copr: jscotka/requre
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.
Note: Once the new tmt is released and deployed in the pipeline the following should be enough:
prepare: how: install copr: jscotka/requre
@psss does that mean that this enable copr repo and packages could be part of L1 metadata? in that case it could be great.
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.
No, currently this is only L2. And teemtee/tmt/issues/647 allows to enable copr repo without specifying a package.
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.
That is great news.
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.
@jscotka Why don't you use Copr repo made by Packit:
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 is missing rhel/epel builds, what is the reason why I'm using COPR, in fedora there is requre already packed.
@jscotka Why don't you use Copr repo made by Packit:
* https://copr.fedorainfracloud.org/coprs/packit/packit-requre-main/
there is missing rhel/epel builds, what is the reason why I'm using COPR, in fedora there is requre already packed.
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.
fixed by packit/requre#174
Build failed.
|
Build failed.
|
Build failed.
|
596373f
to
9ad88b4
Compare
Build succeeded.
|
Build failed.
|
Build failed.
|
Build succeeded.
|
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 as long as tests are passing on el8
this is definitely an improvement to how we test packit in Fedora: going from STI to FMF
Edit: packit-stg/testing-farm-epel-8-x86_64 — Tests passed ...
nice
if ( | ||
hasattr(shutil.copytree, "__defaults__") | ||
and len(shutil.copytree.__defaults__) >= 5 | ||
): |
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 can be done pretty nicely with the inspect module: https://docs.python.org/3/library/inspect.html#inspect.getargspec
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 know, but I don't want to import inspect
module there. And this seems to be sufficient. Especially as I've met more troubles with inspect module when using various version of Python.
tests/conftest.py
Outdated
def tmp_path(): | ||
Path(TMP_DIR).mkdir(exist_ok=True, parents=True) | ||
return Path(tempfile.mkdtemp(prefix=TMP_DIR)) |
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 worry this won't work because tmp_path fixture provides unique paths for tests while this one is static for all - we'll see in the test results.
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 have no idea if fixture code is executed for every usage in def ...(tmp_path)
But I hoped so :-)
so seems to work now somehow
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.
scope="function"
, which means to create the fixture for each test function, is the default, but you could specify it to be more clear.
tests/conftest.py
Outdated
@@ -41,6 +43,21 @@ | |||
) | |||
|
|||
|
|||
TMP_DIR = "/tmp/pytest_tmp_path/" |
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 seems to be used only in the tmp_path
fixture bellow, so could be made local to that function, no need to stay in the module scope.
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.
Looks good to me. Thanks for taking a look at that!
tests/integration/test_patches.py
Outdated
@pytest.mark.skipif( | ||
not check_copytree_dirs_exists_support(), | ||
reason="Old python version does not support copytree exists dirs parameter", | ||
) |
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.
Isn't this #1160 ?
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.
right it is exactly this issue, so it will be fixed/(test skipped) by this PR
Build succeeded.
|
Tested now also locally, so it should now work in fedora dist-git: