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.

Sign up for GitHub

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

Jump to bottom

DEBUG-2334 fix running documentation with scenario #3467

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

DEBUG-2334 fix running documentation with scenario #3467

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/execute/run.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@
If the test contains `@scenarios.SCENARIO_NAME` such as `@scenarios.integrations`, then the `./run.sh` needs to be adjusted to the following:

```bash
./run.sh SCENARIO_NAME tests/path_to_test.py
./run.sh +S SCENARIO_NAME tests/path_to_test.py

# 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 !

```

## Run only one class, or one method
Expand Down
Loading