[RHELC-1606] Fix of multiple backup of one file #1267
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1267 +/- ##
==========================================
- Coverage 95.83% 92.01% -3.83%
==========================================
Files 55 55
Lines 4756 4757 +1
Branches 836 836
==========================================
- Hits 4558 4377 -181
- Misses 112 284 +172
- Partials 86 96 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| # Remove the original file | ||
| try: | ||
| os.remove(original_file_path) | ||
| removed_paths.append(original_file_path) | ||
| # FileNotFound on Python 3+, due compatibility with Python 2.7 using OSError | ||
| except OSError: | ||
| # If the path is present multiple times in the 'rpm -Va' output | ||
| # it's possible, the path was already removed. If not, that's fail. | ||
| if original_file_path not in removed_paths: | ||
| assert False |
There was a problem hiding this comment.
Can we use with pytest.raises(OSError):? What about IOError?
There was a problem hiding this comment.
@Venefilyn I checked this and the comments ### to have this prepared. So both of the commits are ready and we can discuss which we will merge.
Comments are updated, but I don't think this can be changed. I'm also checking if the path was really removed and the exception doesn' have to be raised each time.
| @@ -297,37 +304,45 @@ def test_backup_package_file_complete( | |||
| backup_package_files_action, | |||
| global_backup_control, | |||
| ): | |||
| # Prepare the rpm -va ouput to the PRE_RPM_VA_LOG_FILENAME file | |||
| ### Prepare the 'rpm -Va' ouput to the PRE_RPM_VA_LOG_FILENAME file | |||
There was a problem hiding this comment.
Since I'm a bit unclear about the triple hashes. What is the significance of # vs ###?
There was a problem hiding this comment.
Probably just some typo or re-commenting the same comment two more times
There was a problem hiding this comment.
I thought so too but then saw a comment else where in the PR with
### Prepare the files from the 'rpm -Va' output by creating them
# Use only unique paths, since one path can be present multiple times in the output
# https://issues.redhat.com/browse/RHELC-1606There was a problem hiding this comment.
The plan was to create some headings to each part of the test. But I think it wasn't good plan :D I'll remove that, since it's unclear
There was a problem hiding this comment.
No it's fine but was curious of the reason why. Wonder if we can make it more clear somehow..
I think when it's like this of a heading and comments underneath I'd go with this or something like it
# Heading
#
# Text here
# Text here
Venefilyn
added
the
tests/tier0
|
/packit test --labels tier0 |
|
@hosekadam I had forgotten that I even reviewed this, but I created a solution too without realizing we had a solution created already. What is stopping us from doing something like this? |
|
@Venefilyn looking at it, I would say they are really similar. Maybe the mine solution is easier to read (but it's probably because I did that :D) - like just adding the backed up path to the list of already backed up paths (which we already use, just extending it). And yours where we have in the paths to back up just unique paths. Maybe the memory usage will be lower in this case. Just one case came to my mind - what if the status or type of file would differ, but the path would be the same? In that case it would be added to the list, right? But maybe this is a too much corner case... But I would definitely use the unit tests from this PR, which checks the complete process of the backup and rollback, I think the unit test is good because it tests the real usage (and I spent some time on it :D). But I understand, with your solution it's probably not needed so much. |
hosekadam
force-pushed
the
fix-multiple-file-backup
branch
2 times, most recently
from
eea36c4 to
5a049c4
Compare
|
/packit test --labels tier0 |
I would merge the two PRs in this one. I prefer @Venefilyn solution as it is simpler and does not require always updating a list after each new backed up file. Ideally, we should receive the data in that function already prepared to parse and backup. But I agree with @hosekadam to keep his unit_tests as it is testing the whole situation. I would try to look for a way to split the test or simplify if possible. The test itself is very difficult to read 🤔. My vote here is then to pick @Venefilyn solution and @hosekadam unit_tests. |
File can be present multiple times in the output of 'rpm -Va' command. The file is backed up only once.
hosekadam
force-pushed
the
fix-multiple-file-backup
branch
from
5a049c4 to
0269506
Compare
Taken from Adam's PR oamg#1267 Co-authored-by: Adam Hosek <[email protected]>
Taken from Adam's PR oamg#1267 Co-authored-by: Adam Hosek <[email protected]> Signed-off-by: Freya Gustavsson <[email protected]>
Taken from Adam's PR oamg#1267 Co-authored-by: Adam Hosek <[email protected]> Signed-off-by: Freya Gustavsson <[email protected]>
Taken from Adam's PR oamg#1267 Co-authored-by: Adam Hosek <[email protected]> Signed-off-by: Freya Gustavsson <[email protected]>
Taken from Adam's PR oamg#1267 Co-authored-by: Adam Hosek <[email protected]> Signed-off-by: Freya Gustavsson <[email protected]>
Taken from Adam's PR oamg#1267 Co-authored-by: Adam Hosek <[email protected]> Signed-off-by: Freya Gustavsson <[email protected]>
Taken from Adam's PR #1267 Co-authored-by: Adam Hosek <[email protected]> Signed-off-by: Freya Gustavsson <[email protected]>
Taken from Adam's PR #1267 Co-authored-by: Adam Hosek <[email protected]> Signed-off-by: Freya Gustavsson <[email protected]>
|
We decided to merge this and my PR so closing this one |
File can be present multiple times in the output of 'rpm -Va' command. The file is backed up only once.
Also, the unit test was updated to handle this situation.
Jira Issues:
Checklist
[RHELC-]or[HMS-]is part of the PR titleRelease Pendingif relevant