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

DEBUG-2334 fix running documentation with scenario #3467

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

p-datadog
Copy link
Contributor

Motivation

Current documentation mixes upper and lower case for scenario name in the instructions for running a single file with a scenario.

Changes

Change the instructions to use +S option to pass the actual scenario name as specified in the test file (in lower case). Now the instructions can be followed as written.

Workflow

  1. ⚠️ Create your PR as draft ⚠️
  2. Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
  3. Mark it as ready for review
    • Test logic is modified? -> Get a review from RFC owner. We're working on refining the codeowners file quickly.
    • Framework is modified, or non obvious usage of it -> get a review from R&P team

🚀 Once your PR is reviewed, you can merge it!

🛟 #apm-shared-testing 🛟

Reviewer checklist

  • If PR title starts with [<language>], double-check that only <language> is impacted by the change
  • No system-tests internal is modified. Otherwise, I have the approval from R&P team
  • CI is green, or failing jobs are not related to this change (and you are 100% sure about this statement)
  • A docker base image is modified?
    • the relevant build-XXX-image label is present
  • A scenario is added (or removed)?

The example was mixing upper and lower case scenario name.
Use +S option to pass the actual scenario name as specified
in the test file (in lower case).
@p-datadog p-datadog requested a review from a team as a code owner November 14, 2024 16:23

# Example: for @scenarios.integrations in tests/integrations/test_sql.py
./run.sh INTEGRATIONS tests/integrations/test_sql.py
./run.sh +S integrations tests/integrations/test_sql.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

Scenario names are in upper case in bash commands, and lowecase in python.

Furthermore, the first argument is interpreted as the senario if it's in uppercase (that the prupose to keep it uppercase), in order to avoid to write the +S.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get that the usage of uppercase might make some operations more convenient for those developers that use system tests all the time. However, since it is not possible to know which case to use when (you need to somehow have this knowledge already), using two different cases hampers discoverability and creates problems when searching (since both cases need to be checked).

The instructions, as written, say that scenario name is the value in the decorator in the test code. They do not mention the required transformation to upper case in order to use the scenario name in (some? all?) shell commands.

This PR proposes using the same case everywhere. This is already supported by the code and is easy to understand for newcomers to the project - the target audience, one would assume, for the instructions in question.

Alternatively, we could add the text of your comment to the instructions, but 1) the instructions would get much longer, 2) they would also take more time to understand, 3) it would still be impossible to copy-paste scenario name from a test source file to the terminal and have the test be correctly executed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I understand.

This PR proposes using the same case everywhere.

So this PR is not only a doc improvment, but a suggestion to change an usage.

I tend agree with your rationals. But the fact we use an uppercase identifier as scenario name without the need of +S is already known by all users. I actually presume, that almost nobodies knows +S. Changing that needs a proper communication, and verify that it's something users will be confortable with.

The starting point for that is to initiate the discussion in #apm-shared-testing channel. If they're ok, we'll keep the possibility of ./run.sh UPPERCASE as it's compatible. So the question you can ask can be something like :

I propose to change the doc to only mention ./run.sh +S lowercase as the official way to run a system-tests scenario because <---your rationnals--->.
Though, using ./run.sh UPPERCASE will always be possible to keep backward compabitility.

If we receive positive feedback, all good !

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