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

Update e2e-testing.md #105

Closed
wants to merge 2 commits into from
Closed

Update e2e-testing.md #105

wants to merge 2 commits into from

Conversation

chrisleewilcox
Copy link

@chrisleewilcox chrisleewilcox commented Aug 30, 2024

Discussed the use of the term 'should' at the start of test names. Using 'should' at the start of test names is a long standing and accepted standard. Our guidelines should be more aligned with the tools and frameworks guidelines and recommendations that we use.

Even repeated test names within a test file using 'should' at the start of test names should be considered valid.

The test framework we use implies all test names start with 'it' which makes 'should' coming after 'it' a more readable, expressive, and consistent practice.

Discussed the use of the term 'should' at the start of test names.  Using 'should' at the start of test names is a long standing and accepted standard.  We should be more aligned with the tools and frameworks guidelines and recommendations that we use.

Even repeated test names within a test file using 'should' at the start of test names should be considered valid.
@chrisleewilcox chrisleewilcox requested a review from a team as a code owner August 30, 2024 16:36
@hjetpoluru hjetpoluru self-requested a review August 30, 2024 18:06
hjetpoluru
hjetpoluru previously approved these changes Aug 30, 2024
Copy link

@hjetpoluru hjetpoluru left a comment

Choose a reason for hiding this comment

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

LGTM

```

❌ Test name should be completely avoided: The use of a `should` prefix and the word `and` can decrease the readability of the test,making it harder to understand what the test is doing as well as diagnose and fix issues.
❌ Test name should be completely avoided: Long test names that may over explain the test intention and the word `and` can decrease the readability of the test,making it harder to understand what the test is doing as well as diagnose and fix issues.
Copy link
Contributor

@desi desi Sep 5, 2024

Choose a reason for hiding this comment

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

I believe the reason this was written this way originally was because this was a pattern we were seeing and wanted to call out specifically that we don't want to continue using that pattern.

Copy link
Author

@chrisleewilcox chrisleewilcox Sep 5, 2024

Choose a reason for hiding this comment

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

The pattern of using 'should' at the beginning of a test description is to make the test worded in a manner that if spoken is sounds right and makes sense. It is a very common QA industry practice when using BDD test framework for test development. This practice is not a preference but more of a mechanism, or method, to easily name test without having to think too much about it. When the methods becomes a habit then QA engineers would just always invoke that habit even when it may not be needed in all or even most cases. Think of it like 'it should' at the beginning is compatible with all test naming/describing and that's why it's widely used.

Copy link
Author

@chrisleewilcox chrisleewilcox Sep 5, 2024

Choose a reason for hiding this comment

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

The edit was to adjust from saying never to use 'should' and to emphasize clear what the test is intended. But never use 'should' may be a little extreme to be applied in all cases as there are clear examples of using 'should' just sounds the most appropriate for a test.

We can adjust to say something like....

Although 'should' is a widely used method at the begining for naming test it does not mean it needs to be always used and it's encouraged to name test in any manner that makes the test understandable. Some teams may have a preference for naming conventions so check with teams on desired approach.

Copy link
Author

Choose a reason for hiding this comment

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

Here is a prompt that may explain better the use of a common practice for naming test.

For BDD test development, why is 'should' commonly used at the start of it()?
image

Copy link
Contributor

@desi desi Sep 6, 2024

Choose a reason for hiding this comment

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

@chrisleewilcox I come from old school Ruby world / rspec so I am very familiar with BDD. My main concern here was that I knew this call out was very deliberate and I didn't want it to change without some additional discussion. I believe @mcmire was one of the folks that felt strongly about this and he is out on paternity leave. He may have additional thoughts when he returns but for now I do really like the rewording you included in the comment above. I think the new wording is a nice balance, essentially "don't use it just because you are in the habit of it but feel free to use it if it actually makes the test easier to read"

@Gudahtt do you have additional thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

@desi thanks for the feedback. Spoke with some QA team members and some of the responses were related to test output that didn't make sense outside of QA engineers. Now that I have a better understanding of the history and reasoning I have no problem leaving the doc as is.

I'll just add @Gudahtt recommended change for proper spacing.

@Gudahtt
Copy link
Member

Gudahtt commented Sep 5, 2024

This was a deliberate choice to make test descriptions more concise. By re-framing the description to say what a test does, rather than what it should do, you convey the same information with fewer words.

e.g.

it('should show a warning when a malicious transaction is suggested')

versus

it('shows a warning when a malicious transaction is suggested')

docs/e2e-testing.md Outdated Show resolved Hide resolved
add needed space

Co-authored-by: Mark Stacey <[email protected]>
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.

4 participants