Skip to content

Feat all child steps #10

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

apatelia
Copy link
Contributor

  • Report all child steps in extra property of a step
  • Include hook, expect, pw:api and test.step steps by default
  • Add a new configuration option - testStepsOnly
  • Allow existing behavior to report only 'test.step' steps through 'testStepsOnly' configuration
  • Add a step's category, duration and location in extra property of the step
  • Update README to reflect configuration and feature updates
  • Add 'examples' document for testStepsOnly configuration option

BREAKING CHANGE:
Include hook, expect, pw:api and test.step steps by default, but is configurable to allow for old behavior of including only test.step.

- Report all child steps in extra property of a step
- Include hook, expect, pw:api and test.step steps by default
- Add a new configuration option - testStepsOnly
- Allow existing behavior to report only 'test.step' steps through 'testStepsOnly' configuration
- Add a step's category, duration and location in extra property of the step
- Update README to reflect configuration and feature updates
- Add 'examples' document for testStepsOnly configuration option

BREAKING CHANGE:
Include hook, expect, pw:api and test.step steps by default
@apatelia
Copy link
Contributor Author

I have tested it against three of my public test repositories. All have unique styles of step implementations.

  1. Repo 1 - Standard Playwright styled tests
  2. Repo 2 - BDD styled tests using steps implementation
  3. Repo 3 - BDD styled tests using typescript decorators for steps implementation

@Ma11hewThomas Ma11hewThomas self-assigned this Oct 27, 2024
@Ma11hewThomas Ma11hewThomas self-requested a review October 27, 2024 18:01
@apatelia
Copy link
Contributor Author

Fixes #9

@Ma11hewThomas
Copy link
Collaborator

Hello @apatelia, thank you for the contribution.

After testing the changes, we’d like to suggest a few adjustments.

Across various test suites, we’ve observed that the default configuration has led to a significant increase in report size, with some reports being approximately 5 times larger in file size and containing 10 times more lines.

To address this, we suggest the following configuration options:

Option Default Description
steps false Include steps in your report (includes only test steps by default)
testStepsOnly true When steps is enabled, include only test-specific steps

This means that, for users on the default configuration, there won't be a significant change in their reports unless they choose to include all steps explicitly.

Does this approach align with your expectations and meet the needs of users using BDD?

@apatelia
Copy link
Contributor Author

apatelia commented Nov 4, 2024

Thanks @Ma11hewThomas! It sounds good to me. I will implement it, test it, and then update this PR.

Copy link
Collaborator

@Ma11hewThomas Ma11hewThomas left a comment

Choose a reason for hiding this comment

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

Hello @apatelia, thank you for the contribution.

After testing the changes, we’d like to suggest a few adjustments.

Across various test suites, we’ve observed that the default configuration has led to a significant increase in report size, with some reports being approximately 5 times larger in file size and containing 10 times more lines.

To address this, we suggest the following configuration options:

Option Default Description
steps false Include steps in your report (includes only test steps by default)
testStepsOnly true When steps is enabled, include only test-specific steps

This means that, for users on the default configuration, there won't be a significant change in their reports unless they choose to include all steps explicitly.

Does this approach align with your expectations and meet the needs of users using BDD?

@apatelia
Copy link
Contributor Author

Hello @Ma11hewThomas, sorry that it took me unexpectedly long to respond to the original change request. I appreciate the testing you have done and the valid point that you have raised about the report being too big. But I think we do not need to introduce additional steps configuration variable as you have suggested. Because having two configuration variables allow for total 4 permutations, and I think it may result into a report with unintended details (i.e. bugs with the reporter).

steps testStepsOnly
Permutation 1 true true
Permutation 2 true false
Permutation 3 false true
Permutation 4 false false

I think that setting the testStepsOnly to true by default, we will achieve the desired effect. What do you think?

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.

2 participants