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

Add sslib master tox build + flesh out test docs #915

Merged
merged 2 commits into from
Sep 18, 2019

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Sep 16, 2019

Fixes issue #:
None. Suggested by @joshuagl in #913

Description of the changes being introduced by the pull request:
Add a tox build that runs tests against securesystemslib's tip of development, i.e. its master branch, to ease preparation of tuf for a new securesystmeslib release.

The tox build is run on travis but is allowed to fail.

This PR also fleshes out the testing section of the contribution documentation.

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

@lukpueh lukpueh force-pushed the tox-with-sslib-master branch from 08b0f4f to 81ff59e Compare September 16, 2019 16:59
Add a tox build that runs tests against securesystemslib's tip of
development, i.e. master branch, to ease preparation of tuf for a
new securesystmeslib release.

The tox build is run on travis but is allowed to fail.

This commit also fleshes out the testing section of the
contribution documentation.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@lukpueh lukpueh force-pushed the tox-with-sslib-master branch from 81ff59e to 593490d Compare September 16, 2019 17:04
@joshuagl
Copy link
Member

Oh, this looks great. Thanks @lukpueh

@lukpueh
Copy link
Member Author

lukpueh commented Sep 17, 2019

@aditya, would you mind taking a quick look at this PR, especially the updated "Testing" section of the contribution doc?

$ python aggregate_tests.py


To run the tests, measuring code coverage, the script can be run with the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should coverage be added to dev-requirements.txt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good thinking! It probably should be, although dev-requirements.txt pins all dependencies, and that IMHO doesn't make sense for a development tool.
Maybe we can adopt a requirements hierarchy similar to the one proposed in in-toto/in-toto@06a2898. I'd leave that for another PR though.

As a temporary fix we could either add it to dev-requirements.txt pinned or unpinned (maybe with a TODO comment), or add a note to CONTRIBUTORS.rst, telling contributors to install it separately when running tests with coverage. I tend towards the second for above reasons. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, the second option is better as sometimes temporary fixes have a bad habit of becoming permanent!

Add hint to install `coverage` before using it.

This should be installed via dev-requirements.txt, however it
does not seem to fit in there, because dev-requirements.txt pins
all its dependencies which does not seem to make sense for a
development tool.

Maybe a hierarchy of requirements.txt similar to
in-toto/in-toto@06a2898
could be established.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@lukpueh
Copy link
Member Author

lukpueh commented Sep 18, 2019

Thanks for the review, @aditya!

@lukpueh lukpueh merged commit f79ee33 into theupdateframework:develop Sep 18, 2019
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