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

Remove pulp smash #851

Merged
merged 1 commit into from
Aug 16, 2023
Merged

Remove pulp smash #851

merged 1 commit into from
Aug 16, 2023

Conversation

hstct
Copy link
Contributor

@hstct hstct commented Jul 24, 2023

closes #796

@hstct
Copy link
Contributor Author

hstct commented Jul 24, 2023

depends on #776 and #847

@hstct hstct force-pushed the remove-pulp-smash branch 2 times, most recently from f316380 to 11e5435 Compare July 25, 2023 07:26
hstct
hstct commented Jul 25, 2023
View reviewed changes
functest_requirements.txt Outdated Show resolved Hide resolved
@hstct hstct force-pushed the remove-pulp-smash branch 12 times, most recently from e55d747 to da31265 Compare July 27, 2023 13:54
@hstct hstct marked this pull request as ready for review July 27, 2023 13:55
@hstct hstct force-pushed the remove-pulp-smash branch 6 times, most recently from 5abf0db to 3468545 Compare July 28, 2023 13:32
@hstct hstct mentioned this pull request Jul 28, 2023
Merged
@hstct
Copy link
Contributor Author

hstct commented Jul 28, 2023

Depends on this pulpcore PR: pulp/pulpcore#4177

@hstct
Copy link
Contributor Author

hstct commented Jul 31, 2023

Needs a new pulpcore release before the tests will work.

@hstct hstct added the .misc CHANGES/<issue_number>.misc label Aug 9, 2023
@hstct hstct added the Tests label Aug 9, 2023
@hstct hstct mentioned this pull request Aug 10, 2023
Merged
quba42
quba42 approved these changes Aug 16, 2023
View reviewed changes
Copy link
Collaborator

@quba42 quba42 left a comment

Choose a reason for hiding this comment

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

I did not fully understand the changes to verify_publication_data, and I don't fully understand why new pytest fixtures go in the files they go in as well as their naming scheme.

Putting aside the above caveats, these changes look sane to me, and the tests are green, and my gut feeling is that it makes more sense to get these refactors in and continue working on top, than it does to endlessly review them line by line, while continuing to work with the old test suit.

@hstct
Copy link
Contributor Author

hstct commented Aug 16, 2023

I did not fully understand the changes to verify_publication_data, and I don't fully understand why new pytest fixtures go in the files they go in as well as their naming scheme.

It combines two steps instead of getting the content and then list it for every type it will now do in one go. I thought it would help the readability of the the fixture but maybe I made it more obscure? Happy to change it again though.

@hstct
Copy link
Contributor Author

hstct commented Aug 16, 2023

On a second thought we can just merge it and if its causing too much confusion later on it is simple to change that wouldn't effect much.

@hstct hstct merged commit 5cb3bf5 into pulp:main Aug 16, 2023
8 checks passed
@hstct hstct deleted the remove-pulp-smash branch August 16, 2023 09:14
@quba42 quba42 mentioned this pull request Aug 30, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quba42 quba42 quba42 approved these changes

Assignees
No one assigned
Labels
.misc CHANGES/<issue_number>.misc Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove pulp-smash dependency
2 participants
@hstct @quba42