-
Notifications
You must be signed in to change notification settings - Fork 882
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
doc: improve integrations testing configuration instructions #5556
doc: improve integrations testing configuration instructions #5556
Conversation
d1d1fe2
to
74d4640
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some great updates here, thanks! I left some comments inline. Additionally, there are a few things you didn't write, but when reading the page, I noticed should be updated.
Platform-specific marks can be used to limit tests to particular platforms.
This is no longer true. Can we remove it? Related, would you be able to add a section about skipping tests by platform or OS? We used to use marks for this, but that got really complicated, so now we use @pytest.mark.skipif(...)
with things imported from releases.py.
E.g.,
from tests.integration_tests.releases import CURRENT_RELEASE, NOBLE
...
@pytest.mark.skipif(CURRENT_RELEASE < NOBLE, reason="blah blah blah")
...
Also, in the image selection section, <image_id>[::<os>[::<release>]]
should now be <image_id>[::<os>::<release>::<version>]
.
``pytest`` arguments may also be passed. For example: | ||
``pytest`` arguments may also be passed. | ||
|
||
Or, specify a path to run all tests inside the given file or directory: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need 3 separate examples here showing how to run a test. One of the benefits of using pytest and tox is that things "just work" like they do for any other pytest or tox project. I think one example here would suffice with perhaps a link to https://docs.pytest.org/en/7.1.x/how-to/usage.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I might be swayed by this, but still think it is worth directly calling out, because I think specifying a path is one of the most common use cases for integration tests. Rarely will a new contributor be running the entire integrations sweet due to the insanely long run time?
Let me see how this looks in tabular form so it takes up less space and maybe this will feel less like a waste of space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So worst comes to worst, I agree, 3 different examples is not necessary and just one command showing how to specify a path would suffice.
@TheRealFalcon Would you be able to elaborate what you mean by this? Isn't the following an example of this in action? Once I have some clarity on this, I agree that it would be valuable to expand further on how to develop new integration tests / modify existing ones. (this is actually my next documentation task) |
See this example from 22.3. We now just use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more minor things I saw on another read through, and then we should be good!
@TheRealFalcon made all changes as requested. hopefully this is good to go! |
on of: | ||
|
||
- ``azure``: Microsoft Azure | ||
- ``ec2``: Amazon EC2t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, a t
got added Amazon EC2
. After this one we should be really actually for sure good 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha great catch. thanks @TheRealFalcon !
7fd92c3
to
a08b7b3
Compare
Proposed Commit Message
Merge type