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

feat: add more testing goodies (screenshot, video, tracing) #70

Merged
merged 2 commits into from
Aug 8, 2021

Conversation

mxschmitt
Copy link
Member

@mxschmitt mxschmitt commented Jul 30, 2021

Fixes ##62

@mxschmitt mxschmitt force-pushed the feature/add-more-testing-goodies branch from 68c8da8 to aa6fff1 Compare August 2, 2021 10:36
Copy link

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

Let's wait a bit with this to closer follow js version.

pytest_playwright/pytest_playwright.py Show resolved Hide resolved
return os.path.join(output_dir, slugify(request.node.nodeid), folder_or_file_name)


@pytest.fixture()
Copy link

Choose a reason for hiding this comment

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

This was a "session" fixture before - what's the difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

Before: Context options were for the whole test execution
After: Context options are per test. Since we re-create a context for every test, this should be a better handling.
See here for reference: https://docs.pytest.org/en/6.2.x/fixture.html#fixture-scopes

Copy link

Choose a reason for hiding this comment

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

I am a bit worried whether this will affect some users. For example, their custom fixture depends on this one, and now it cannot because of wrong scope? I see changes in our own tests that kind of confirm this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but since this is 0.x we are allowed to do that. Before it was more a wrong implementation from my side.

) -> Dict:
context_args = {}
if device:
context_args.update(playwright.devices[device])
base_url = pytestconfig.getoption("--base-url")
if base_url:
context_args["base_url"] = base_url

video_option = pytestconfig.getoption("--video")
Copy link

Choose a reason for hiding this comment

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

So, there is a command line argument? I don't see any docs changed, could you please add some documentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

We keep the documentation in the upstream repo. Will follow-up once its landed.
see here: https://playwright.dev/python/docs/next/test-runners

pytest_playwright/pytest_playwright.py Show resolved Hide resolved
pytest_playwright/pytest_playwright.py Outdated Show resolved Hide resolved
return os.path.join(output_dir, slugify(request.node.nodeid), folder_or_file_name)


@pytest.fixture()
Copy link

Choose a reason for hiding this comment

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

I am a bit worried whether this will affect some users. For example, their custom fixture depends on this one, and now it cannot because of wrong scope? I see changes in our own tests that kind of confirm this?

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