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

Implemented fetch_tracker to RepositorySimulator #1705

Conversation

kairoaraujo
Copy link
Collaborator

This commit implements a feature in Repository Simulator to
track the fetch calls to the metadata and targets. This feature was
mentioned in PR #1666 that generated issue #1682.
This commit adds RepositorySimulator.fetch_tracker. It also changes
the tests/test_updater_consistent_snapshot.py to use the
fetch_tracker instead of using mock.

It implements a dataclass that stores the calls to fetch metadata
(_fetch_metadata) in fetch_tracker.metadata and targets
(_fetch_targets) in fetch_tracker.targets.

The fetch calls for metadata, and targets are stored as lists.

Signed-off-by: Kairo de Araujo kdearaujo@vmware.com

Please fill in the fields below to submit a pull request. The more information
that is provided, the better.

Fixes #1682

Description of the changes being introduced by the pull request:

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

@kairoaraujo
Copy link
Collaborator Author

I decided to submit a new PR after my big mess on PR #1704, and I have to say sorry.
I addressed all comments done by @sechkova

As mentioned by @sechkova in the PR #1704

Thanks, the trackers look good, I think this is what we needed.
My comments are minor, related to types but also I think one check is missing.

I was wondering if defining a dataclass (or two dataclasses for metadata and targets) instead of using tuples may simplify the expected calls lists. We can set default values and "typing" will be simpler but on the other hand instead of (root, 1) we'll have to write SomeCall(root, 1) ... so I am not sure about this. This is a way to say that I am looking for a second opinion, not requesting changes 😁 We can keep it like this for now, maybe when more use cases come this choice we'll be more obvious.

I also thought to have different dataclasses for metadata and targets in the initial design.

@coveralls
Copy link

coveralls commented Dec 3, 2021

Pull Request Test Coverage Report for Build 1553255010

  • 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.581%

Totals Coverage Status
Change from base Build 1549318135: 0.0%
Covered Lines: 4032
Relevant Lines: 4116

💛 - Coveralls

@ivanayov
Copy link
Collaborator

ivanayov commented Dec 3, 2021

Looks good to me, just one question

Copy link
Contributor

@sechkova sechkova left a comment

Choose a reason for hiding this comment

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

Looks ok to me. Do you mind squashing the two commits before merging?

@kairoaraujo kairoaraujo force-pushed the issue#1682/repositorysimulator-fetch-tracker branch from ef4eefc to 294b7ea Compare December 3, 2021 17:32
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.

Looks ok to merge to me but I did leave two comments. I can merge as is but maybe have a look and let me know what you think?

tests/test_updater_consistent_snapshot.py Show resolved Hide resolved
tests/test_updater_consistent_snapshot.py Outdated Show resolved Hide resolved
@kairoaraujo kairoaraujo force-pushed the issue#1682/repositorysimulator-fetch-tracker branch from 294b7ea to b9b05dc Compare December 7, 2021 13:32
@jku
Copy link
Member

jku commented Dec 8, 2021

Oh sorry, one more thing (trying to make life easier for @MVrachev): this PR probably has unused imports that will start failing with #1710 ? Can you check the unittest.mock imports?

Feel free to squash the PR to a single commit if you do end up making a change.

@kairoaraujo
Copy link
Collaborator Author

Thank you @jku, I will pay more attention.

This commit implements a feature in Repository Simulator to
track the fetch calls to the metadata and targets. This feature was
mentioned in PR theupdateframework#1666 that generated issue theupdateframework#1682.
This commit adds RepositorySimulator.fetch_tracker. It also changes
the tests/test_updater_consistent_snapshot.py to use the
fetch_tracker instead of using mock.

It implements a dataclass that stores the calls to fetch metadata
(_fetch_metadata) in fetch_tracker.metadata and targets
(_fetch_targets) in fetch_tracker.targets.

The fetch calls for metadata, and targets are stored as lists.

Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>
@kairoaraujo kairoaraujo force-pushed the issue#1682/repositorysimulator-fetch-tracker branch from 64c51f6 to 35cbc3e Compare December 8, 2021 09:00
@jku
Copy link
Member

jku commented Dec 8, 2021

I think we'll go with this version :) thanks

@jku jku merged commit d3b877b into theupdateframework:develop Dec 8, 2021
@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.

RepositorySimulator: add trackers/counters for downloaded files
5 participants