Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[RHELC-828] Add missing integration tests for single yum transaction #677

Merged
merged 8 commits into from
Feb 16, 2023
Merged

[RHELC-828] Add missing integration tests for single yum transaction #677

merged 8 commits into from
Feb 16, 2023

Conversation

r0x0d
Copy link
Member

@r0x0d r0x0d commented Dec 1, 2022 •
edited by bocekm
Loading

Signed-off-by: Rodolfo Olivieri [email protected]

While testing convert2rhel manually in a vagrant box, I realized that the
conversion stopped in the yum transaction validation phase, generating a
traceback and not doing a proper rollback after the error appeared.

Since the transaction validation is supposed to trigger the rollback in case of
any failures, it was strange that the tool was not behaving the way it should.

As part of #674 that introduced the fix, we left out the integration tests that
were supposed to cover this scenario.

Jira Issue: RHELC-828

Checklist

  • PR meets acceptance criteria specified in the Jira issue
  • PR has been tested manually in a VM (either author or reviewer)
  • Jira issue has been made public if possible
  • [RHELC-] is part of the PR title
  • Code and tests are documented properly
  • The commits are squashed to as few commits as possible (without losing data)
  • When merged: Jira issue has been updated to Release Pending

@r0x0d r0x0d self-assigned this Dec 1, 2022
@codecov
Copy link

codecov bot commented Dec 1, 2022 •
edited
Loading

Codecov Report

Base: 92.69% // Head: 92.71% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (94213f9) compared to base (3210770).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #677      +/-   ##
==========================================
+ Coverage   92.69%   92.71%   +0.02%     
==========================================
  Files          24       24              
  Lines        3273     3282       +9     
  Branches      576      576              
==========================================
+ Hits         3034     3043       +9     
  Misses        172      172              
  Partials       67       67
Flag Coverage Δ
centos-linux-7 87.99% <100.00%> (+0.03%) ⬆️
centos-linux-8 89.00% <75.51%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
convert2rhel/backup.py 98.98% <100.00%> (ø)
convert2rhel/cert.py 95.45% <100.00%> (ø)
convert2rhel/checks.py 90.99% <100.00%> (+0.14%) ⬆️
convert2rhel/main.py 91.13% <100.00%> (ø)
convert2rhel/pkgmanager/handlers/yum/__init__.py 98.00% <100.00%> (+0.08%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@r0x0d r0x0d force-pushed the add-additional-integration-test-for-yum-transactions branch 10 times, most recently from 4adb380 to af5ffa5 Compare December 2, 2022 14:45
@r0x0d r0x0d force-pushed the add-additional-integration-test-for-yum-transactions branch 2 times, most recently from 0968d4f to d64c17e Compare December 2, 2022 16:54
@r0x0d r0x0d force-pushed the add-additional-integration-test-for-yum-transactions branch from d64c17e to 074ae02 Compare December 2, 2022 17:01
@r0x0d r0x0d force-pushed the add-additional-integration-test-for-yum-transactions branch 6 times, most recently from c5b8a9e to bdebb9e Compare December 5, 2022 13:45
@r0x0d r0x0d force-pushed the add-additional-integration-test-for-yum-transactions branch 4 times, most recently from 3c3c575 to 0df16ce Compare December 6, 2022 13:08
@r0x0d r0x0d force-pushed the add-additional-integration-test-for-yum-transactions branch from 8fe2151 to ac4f771 Compare February 6, 2023 16:18
@r0x0d
Copy link
Member Author

r0x0d commented Feb 6, 2023

/packit test

abadger
abadger approved these changes Feb 8, 2023
View reviewed changes
Copy link
Member

@abadger abadger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving as no code has changed since @SpyTec 's review

@r0x0d r0x0d force-pushed the add-additional-integration-test-for-yum-transactions branch from ac4f771 to 5ea8586 Compare February 9, 2023 13:33
r0x0d
r0x0d commented Feb 9, 2023
View reviewed changes
tests/integration/tier0/single-yum-transaction-validation/main.fmf
Comment on lines +22 to +23
- enabled: false
when: distro == centos-8 or distro == oraclelinux-8
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to add a because: here to say why I'm disabling this test for centos-8 and oraclelinux-8, but since it's been a really long time, I don't remember. I will try to get the rationale at some point.

@r0x0d r0x0d force-pushed the add-additional-integration-test-for-yum-transactions branch 5 times, most recently from c1b358e to bfc8255 Compare February 14, 2023 15:39
r0x0d added 4 commits February 14, 2023 16:11
@r0x0d
Add missing integration tests for single yum transaction
789f8f8 
While testing convert2rhel manually in a vagrant box, I realized that the
conversion stopped in the yum transaction validation phase, generating a
traceback and not doing a proper rollback after the error appeared.

Since the transaction validation is supposed to trigger the rollback in case of
any failures, it was strange that the tool was not behaving the way it should.

As part of oamg#674 that introduced the fix, we left out the integration tests that
were supposed to cover this scenario.

Signed-off-by: Rodolfo Olivieri <[email protected]>
@r0x0d
Improve tier0 description with link
cc8f861 
Signed-off-by: Rodolfo Olivieri <[email protected]>
@r0x0d
Fix integration test
873cccd 
Signed-off-by: Rodolfo Olivieri <[email protected]>
@r0x0d
Fix pre-commit
9938034 
Signed-off-by: Rodolfo Olivieri <[email protected]>
@r0x0d r0x0d force-pushed the add-additional-integration-test-for-yum-transactions branch from c1bab5b to 9938034 Compare February 14, 2023 19:11
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 15, 2023
View reviewed changes
...ntegration/tier0/single-yum-transaction-validation/test_single_yum_transaction_validation.py
try:
os.unlink(cert_path)
except Exception as e:
print("Failed to delete %s. Reason: %s" % (cert_path, e))

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

This expression logs [sensitive data (certificate)](1) as clear text. This expression logs [sensitive data (certificate)](2) as clear text.
...ntegration/tier0/single-yum-transaction-validation/test_single_yum_transaction_validation.py
@@ -0,0 +1,100 @@
import os
import shutil

Check notice

Code scanning / CodeQL

Unused import

Import of 'shutil' is not used.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can open a follow-up PR to remove this type of warning later... I don't want to wait for a couple of hours to get results for the 3rd time in a row.

@bocekm bocekm merged commit b90f5bb into oamg:main Feb 16, 2023
@r0x0d r0x0d deleted the add-additional-integration-test-for-yum-transactions branch February 17, 2023 12:20
@Venefilyn Venefilyn added skip/changelog If it should be excluded from changelog or Release notes. Such as infra, reverted PRs, etc. and removed kind/feature New feature or request labels Feb 20, 2023
Venefilyn pushed a commit that referenced this pull request Jun 19, 2023
@r0x0d @bocekm
@pre-commit-ci @Venefilyn
[RHELC-828] Add missing integration tests for single yum transaction (#…
aef41cf 
…677)

* Add missing integration tests for single yum transaction

While testing convert2rhel manually in a vagrant box, we've bumped into a
traceback when the conversion stopped in the yum transaction validation
phase and no rollback happened.

Since the transaction validation is supposed to trigger the rollback in case of
any failures, it was strange that the tool was not behaving the way it should.

As part of #674 that introduced a fix for that, we left out the integration test
that was supposed to cover this scenario.

Signed-off-by: Rodolfo Olivieri <[email protected]>
Co-authored-by: Michal Bocek <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abadger abadger abadger approved these changes

@bocekm bocekm bocekm approved these changes

@Venefilyn Venefilyn Venefilyn left review comments

@danmyway danmyway danmyway left review comments

Assignees

@r0x0d r0x0d

Labels
skip/changelog If it should be excluded from changelog or Release notes. Such as infra, reverted PRs, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@r0x0d @abadger @Venefilyn @danmyway @bocekm