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

Adds strum as a integration test requirement. #423

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

Superhepper
Copy link
Collaborator

-Adds integration-test feature in order to conditionally add
dependencies.

-Updates the TpmFormatZeroWarning so it is possible to iterate over all
the items when creating integration tests.

@Superhepper
Copy link
Collaborator Author

Was it something like this that you had in mind @wiktor-k ?

@wiktor-k
Copy link
Collaborator

Yeah the test code got significantly smaller. There's a slight cost of setup that I think we'd pay only once and then all similar tests can just reuse what you did here. 👍

I see that the CI complains about MSRV thought :/

-Adds integration-test feature in order to conditionally add
 dependencies.

-Updates the TpmFormatZeroWarning so it is possible to iterate over all
 the items when creating integration tests.

Signed-off-by: Jesper Brynolf <[email protected]>
@Superhepper
Copy link
Collaborator Author

I see that the CI complains about MSRV thought :/

That was just because I thought I was going to be clever and update a dependency.

@Superhepper Superhepper marked this pull request as ready for review July 30, 2023 15:37
Copy link
Collaborator

@wiktor-k wiktor-k left a comment

Choose a reason for hiding this comment

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

I think it looks good. 👍

I wonder how many tests could be improved using this pattern now 😁

Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

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

This looks like a pretty useful pattern, might be good to deploy for other cases too.

@ionut-arm ionut-arm merged commit 6ad4b5f into parallaxsecond:main Aug 2, 2023
10 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.

3 participants