Skip to content

Conversation

@lemontree210
Copy link
Member

@lemontree210 lemontree210 commented Jan 30, 2023

adds tests for functionality introduced in #3501 that resolved #3496

the tests are meant to only run in GH actions before building docs. They are not included in the main test suite, one of the reasons being that Python 3.9+ is required to run the admonition generator.

@lemontree210 lemontree210 marked this pull request as ready for review January 30, 2023 18:32
@lemontree210 lemontree210 self-assigned this Jan 30, 2023
@lemontree210 lemontree210 linked an issue Jan 30, 2023 that may be closed by this pull request
@lemontree210 lemontree210 added the ⚙️ documentation affected functionality: documentation label Jan 30, 2023
@lemontree210 lemontree210 added this to the v20.1 milestone Jan 30, 2023
@lemontree210 lemontree210 added the ⚙️ tests affected functionality: tests label Jan 30, 2023
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

Left two nitpicks, but the rest LGTM :) Nice follow-up PR!

@harshil21 harshil21 self-requested a review February 1, 2023 20:37
Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

great. Might I also suggest:

  • Moving this test to tests/docs/ for better organization (and in spirit of #3420)
  • Running this test along with GH test documentation build, and excluding this test from the regular test workers.

@lemontree210
Copy link
Member Author

great. Might I also suggest:

* Moving this test to `tests/docs/` for better organization (and in spirit of #3420)

* Running this test along with GH test documentation build, and excluding this test from the regular test workers.

Done. This is the first time I ever changed a GH action, but looks like it works :)

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

Can you maybe add a README.md to tests/docs/ stating that

  • that dir contains tests that cover our own scripting logic for the docs
  • running pytest usually doesn't include that due to the missing test_ prefix

?

@lemontree210
Copy link
Member Author

Can you maybe add a README.md to tests/docs/

Done

@Bibo-Joshi Bibo-Joshi merged commit aa14cb5 into doc-fixes Feb 2, 2023
@Bibo-Joshi Bibo-Joshi deleted the admonition-tests branch February 2, 2023 18:27
@Bibo-Joshi Bibo-Joshi mentioned this pull request Feb 2, 2023
9 tasks
Bibo-Joshi added a commit that referenced this pull request Feb 5, 2023
…3515, #3523,  #3498, #3529)

Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
Co-authored-by: Shivam Saini <51438830+shivamsn97@users.noreply.github.com>
Co-authored-by: Aditya Yadav <adityayadav11082@gmail.com>
Co-authored-by: Dmitry Kolomatskiy <58207913+lemontree210@users.noreply.github.com>
Co-authored-by: Crsi <47722349+CrsiX@users.noreply.github.com>
Co-authored-by: poolitzer <github@poolitzer.eu>
Co-authored-by: Aditya <clot27@apx_managed.vanilla>
@github-actions github-actions bot locked and limited conversation to collaborators Feb 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

⚙️ documentation affected functionality: documentation ⚙️ tests affected functionality: tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[DOCUMENTATION] Add new admonitions to telegram classes

4 participants