Skip to content

Clarify key rotations test cases by using keywords - #1649

Merged
jku merged 2 commits into
theupdateframework:developfrom
MVrachev:clarify-root-rotations-test
Nov 11, 2021
Merged

Clarify key rotations test cases by using keywords#1649
jku merged 2 commits into
theupdateframework:developfrom
MVrachev:clarify-root-rotations-test

Conversation

@MVrachev

Copy link
Copy Markdown
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

coveralls commented Oct 28, 2021

Copy link
Copy Markdown

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%

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

💛 - Coveralls

Comment threadtests/test_updater_key_rotations.py Outdated
Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev
MVrachevforce-pushed the clarify-root-rotations-test branch from 8f95e2a to beb8087CompareNovember 5, 2021 14:46
@MVrachev

Copy link
Copy Markdown
CollaboratorAuthor

#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 jkuNovember 11, 2021 11:55
jku
jku approved these changes Nov 11, 2021

@jkujku left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
CollaboratorAuthor

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:developNov 11, 2021
@MVrachev
MVrachev deleted the clarify-root-rotations-test branch November 25, 2021 11:26
@lukpuehlukpueh mentioned this pull request Dec 13, 2021
@coveralls

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 1393945892

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.54%

TotalsCoverage Status
Change from base Build 1390970499:1.1%
Covered Lines:3116
Relevant Lines:3129

💛 - Coveralls

1 similar comment
@coveralls

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 1393945892

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.54%

TotalsCoverage Status
Change from base Build 1390970499:1.1%
Covered Lines:3116
Relevant Lines:3129

💛 - Coveralls

Sign up for freeto 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

@MVrachev@coveralls@jku