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

turn doc build warnings into errors #1724

Merged
merged 4 commits into from
Dec 16, 2021

Conversation

kairoaraujo
Copy link
Contributor

This commit adds to the docs tox session the flag -W, which
turns the warnings into errors.

The CI will fail once it gets errors.

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

Linked to #1722

  • 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

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.

While I think this is a good change and we should keep it, we have the docs environment in tox only for local builds. In the CI we use the integrated webhooks from readthedocs.
So in addition to your change, we need to update the .readthedocs.yaml file with the fail_on_warning option: https://docs.readthedocs.io/en/stable/config-file/v2.html#sphinx

@coveralls
Copy link

coveralls commented Dec 13, 2021

Pull Request Test Coverage Report for Build 1586677835

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

Totals Coverage Status
Change from base Build 1586501242: 0.0%
Covered Lines: 4091
Relevant Lines: 4176

💛 - Coveralls

@kairoaraujo
Copy link
Contributor Author

Thank you @sechkova, I see now the failure in the checks.

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.

It makes sense to me.

@jku
Copy link
Member

jku commented Dec 14, 2021

Well CI shows it failing as intended...

I'm not sure about this though: reproducing tests locally is pretty important (sending PRs only to get red lights is demoralizing) and our testing setup is already a bit complicated in that regard: tox -e lint,py is probably currently the easiest way to test 99% of what CI tests. With this commit it becomes tox -e lint,docs,py which is a bit much...

Could we set default envlist = lint,docs,py so developers can just call tox and it does approximately the right thing? or does someone actually have multiple pythons installed they want to test?

The CI already shouldn't need the envlist as I think it it explicitly sets toxenv to py (so uses the current system python)

@kairoaraujo
Copy link
Contributor Author

Could we set default envlist = lint,docs,py so developers can just call tox and it does approximately the right thing? or does someone actually have multiple pythons installed they want to test?

The CI already shouldn't need the envlist as I think it it explicitly sets toxenv to py (so uses the current system python)

+1 to set default envlist = lint,docs,py and make it straightforward for contributors running tox.
I particularly have multiple Python versions installed. Still, I think those with multiple Python versions should explicitly use the tox -e py39,py310 (my case) instead of the default.

@jku
Copy link
Member

jku commented Dec 14, 2021

Yeah that sounds good to me.

build fix is in (#1726), if you rebase you should also get a green build now...

@kairoaraujo kairoaraujo force-pushed the doc_warnings_to_errors branch from aee4963 to 0fe0b8b Compare December 14, 2021 11:05
@lukpueh
Copy link
Member

lukpueh commented Dec 15, 2021

or does someone actually have multiple pythons installed they want to test?

I do, yes. But I'm also fine in changing the way I invoke tox to do it.

Given this change we should also update the note about tox in the contributors docs (see bottom). But we can add that to #1709. @kairoaraujo, would you mind adding a note there?

@kairoaraujo
Copy link
Contributor Author

Given this change we should also update the note about tox in the contributors docs (see bottom). But we can add that to #1709. @kairoaraujo, would you mind adding a note there?

I don't mind adding a note there, but can someone help me with some decisions?

Current doc:

With `tox <https://testrun.org/tox/>`_ the test suite can be executed in a
separate *venv* for each supported Python version. While the supported
Python versions must already be available, ``tox`` will install ``tuf`` and its
dependencies anew in each environment.

    $ tox

My initial suggestion is:

With `tox <https://testrun.org/tox/>`_ the test suite can be executed in a
separate *venv* for the default installed Python version. While the supported
Python version must already be available, ``tox`` will install ``tuf`` and its
dependencies anew.

    $ tox

About the part that mentions tox will install tuf's "dependencies anew in each environment.".
That is true for the first run, but it will not automatically recreate that after it. It will reuse it.

Some suggestions:

  • add a mention if you are changing any of the requirements to use tox -r to recreate the venv
  • OR add the example as tox -r
  • OR add recreate = true to tox.ini have recreation for every run [IMHO -1]

Should I also add a piece of information about using an alternative installed Python version?

Suggestion

Running the test with tox and alternative installed Python version is possible using  ``-e py<version>``.

   $ tox -e py310

@jku
Copy link
Member

jku commented Dec 15, 2021

As general advice: We should aim to make the document (quite a bit) shorter, not longer.

I can try to come up with actual suggestions later but maybe that helps already?

Kairo de Araujo added 4 commits December 16, 2021 10:04
This commit adds to the docs tox session the flag ``-W``, which
turns the warnings into errors.

The CI will fail once it gets errors.

Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>
CI uses the integrated webhooks from readthedocs.

Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>
Simplified ``tox`` environ list not to expect that developer has
all multiple Python version, but instead run with the python version
available.

Also, it adds docs build to environ list.

Running ``tox`` will run the lint, docs, and py.
The CI covers the multiple supported Python versions, and the
developers still can use tox -e py{version}

Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>
The mention of testing using multiple Python versions by default was
removed.

Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>
@kairoaraujo kairoaraujo force-pushed the doc_warnings_to_errors branch from 0fe0b8b to 7dc5940 Compare December 16, 2021 09:06
@kairoaraujo
Copy link
Contributor Author

I added a small and simple suggestion.

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, thanks

@jku jku merged commit 3823fd6 into theupdateframework:develop Dec 16, 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.

5 participants