Skip to content

Tests: download a target with succinct_roles enabled#2031

Merged
jku merged 3 commits intotheupdateframework:developfrom
MVrachev:tap15-download-target-test
Aug 8, 2022
Merged

Tests: download a target with succinct_roles enabled#2031
jku merged 3 commits intotheupdateframework:developfrom
MVrachev:tap15-download-target-test

Conversation

@MVrachev
Copy link
Collaborator

Related to issue #1909

Description of the changes being introduced by the pull request:

Add test downloading a target file when succonct_roles is used and as
such test the whole updater downloading workflow.

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 Jun 17, 2022

Pull Request Test Coverage Report for Build 2516412956

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 remained the same at 97.215%

Totals Coverage Status
Change from base Build 2515189215: 0.0%
Covered Lines: 553
Relevant Lines: 565

💛 - Coveralls

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.

So, I'm happy to merge this, it's better than what we have. However it feels like it's just the delegation graph test test_succinct_roles_graph_traversal with download_target() call added to the end...

Some potential ways to improve:

  • avoid duplicating all delegation graph checks: those checks are complex and I feel like they harm the readability of this test. Maybe it makes sense to verify the delegated role (exp_calls) in the end but even that is debatable
  • actually check that the target content is the expected one, stop using the same content for all targets
  • it's very minimal WRT repository content: e.g. there is never more than one target file in a delegated targets metadata, or even the whole repository. I think we could get more real coverage if we setup a repository with succinct delegation to lots of generated targets (like maybe targets could be every two character combination as file content -- 26*26 files, and filename could be the same as file content), and then the subtests could be a dozen target downloads from that repo?

I'm marking approve, but let me know your opinion

Add test downloading a target file when succonct_roles is used and as
such test the whole updater downloading workflow.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev MVrachev force-pushed the tap15-download-target-test branch from a21e0a1 to 56b14f0 Compare July 20, 2022 16:04
Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev MVrachev force-pushed the tap15-download-target-test branch from 56b14f0 to 7d389f3 Compare July 20, 2022 16:09
@MVrachev
Copy link
Collaborator Author

I rewrote the whole test and used a different approach.
When I decided to follow your advice and generate more target files I realized that I don't need to use the decorator at all anymore.
That's why I instead used for loops which in this case seems like a reasonable choice.

If you don't like that approach I can rever the new commit.

@MVrachev MVrachev requested a review from jku July 21, 2022 08:31
@joshuagl joshuagl changed the title Tests: download a target with succonct_roles enabled Tests: download a target with succinct_roles enabled Jul 27, 2022
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.

I like this, very nice! 1000 files sounds like a lot but if it's not slow I'm not complaining.

Left two comments but this is great as is 👍

@jku
Copy link
Member

jku commented Aug 8, 2022

if it's not slow I'm not complaining.

looking at just results for this CI run, this seems to increase complete test suite runtime by ~50%

Please test this before writing 1000 files.

@jku
Copy link
Member

jku commented Aug 8, 2022

Actually (since martin may be on a holiday) I can push a commit here myself that fixes the minor issues: will merge after that

* move to the test file that contains all the other download tests
* don't write 1000 files: it can be slow in CI
* Compare file content to what was originally written
  (also read the whole file content)
* Remove try-except that seems unused

Signed-off-by: Jussi Kukkonen <jku@goto.fi>
@jku jku force-pushed the tap15-download-target-test branch from 23deb05 to 01b30cc Compare August 8, 2022 15:59
@jku jku merged commit 3308c29 into theupdateframework:develop Aug 8, 2022
@MVrachev
Copy link
Collaborator Author

Thank you for pushing the changes @jku!
I was indeed on a holiday.

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

Comments