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

Clarify key rotations test cases by using keywords #1649

Merged
merged 2 commits into from
Nov 11, 2021

Conversation

MVrachev
Copy link
Collaborator

Description of the changes being introduced by the pull request:

As I mentioned in my comment here #1607 (comment) while I was reviewing #1635 it was a little hard for me to remember the placement of the arguments for RootVersion considering they are 15 root rotations tested and the dataset becomes a larger piece of code.

That's why I suggest using keywords whenever possible to do that without splitting the RootVersion instance creation into two lines.

Signed-off-by: Martin Vrachev mvrachev@vmware.com

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@coveralls
Copy link

coveralls commented Oct 28, 2021

Pull Request Test Coverage Report for Build 1448985910

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.1%) to 98.571%

Totals Coverage Status
Change from base Build 1426058326: 1.1%
Covered Lines: 3116
Relevant Lines: 3129

💛 - Coveralls

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev
Copy link
Collaborator Author

MVrachev commented Nov 5, 2021

#1659 was merged and I updated the pr to use keywords in all test cases.
@jku do you think I should rename res back to results and sigs back to signatures?

@MVrachev MVrachev requested a review from jku November 11, 2021 11:55
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

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

LGTM. Is black going to accept these without complaints when you enable black?

In order to make the `RootVersion` test cases understandable it's
better if we use keywords and don't split the different RootVersions
into multiple lines.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev
Copy link
Collaborator Author

Ohh... I forgot to run black on that change.
It's complaining...
I disabled black with # fmt: off for the test cases and then enabled it again with # fmt: on after them.

@jku jku merged commit 45f69a2 into theupdateframework:develop Nov 11, 2021
@MVrachev MVrachev deleted the clarify-root-rotations-test branch November 25, 2021 11:26
@lukpueh lukpueh mentioned this pull request Dec 13, 2021
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