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.

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

Update and enable tests for transaction replay #1525

Merged
merged 20 commits into from
Jul 16, 2024

Conversation

kontura
Copy link
Contributor

@kontura kontura commented Jul 2, 2024

kontura added 15 commits July 2, 2024 14:21
It checks different but very related things but it is much more
readable and understandable.
…action "last" are`

It removes last uses of the old step and removes it.
- replacing actions were merged into `Replaced`
- `removed` was changed just to `Remove`
- The `replay` command is no longer a subcommand of `history` command
- The `replay` command now accepts a directory instead of a single file
- serialized transaction version bumped to `1.0`
- environments no longer contain environment groups
- groups no longer contain group packages
- group actions now require a `reason`
- updated error messages
Reasons are stored in system state and non-installed packages are not in
system state. This means that for dnf5 its not valid to change reason of
non-installed pkg.

Even if the package is being installed, we cannot change the reason
at the same time (such a transaction has to be created manually).
We could use Group but some group would have to be installed: this is
simpler and it doesn't matter for the purposes of the test.
This must have been broken in dnf4, probably beacuse all installonly
packages have just one reason.
I am not really sure what was the purpose of the `unknown` reason but
dnf5 doesn't support it.

There is reason `None` but package installed by dnf5 cannot have reason
`None`.
@pkratoch pkratoch self-assigned this Jul 2, 2024
Copy link
Contributor

@pkratoch pkratoch left a comment

Choose a reason for hiding this comment

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

Thanks for all the patches!

Based on the comment in rpm-software-management/dnf5#1569 regarding the missing package_types in the json format, I think it would be good to have a test for that as well (either test that it's a required field or test that a default value is used).

Comment on lines +1143 to +1145
# This should fail or there should be at least a warning.
# Reported as https://github.com/rpm-software-management/dnf5/issues/1573
@xfail
Copy link
Contributor

Choose a reason for hiding this comment

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

I was a bit confused by this, because the xfail tests are usually written according to the expected behavior, but here there is the undesired (though currently working) behavior. Based on the discussion in the issue, I think dnf should report an error. Do I understand it correctly? If so, can you please change the expected outcome?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure, updated.

@kontura
Copy link
Contributor Author

kontura commented Jul 15, 2024

Thanks for all the patches!

Based on the comment in rpm-software-management/dnf5#1569 regarding the missing package_types in the json format, I think it would be good to have a test for that as well (either test that it's a required field or test that a default value is used).

That is a good idea.
I have added a new commit with that test.

@pkratoch pkratoch merged commit 1d4d27a into rpm-software-management:main Jul 16, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants