Add screenshot teardown fixture - approach 1

[danielhollas]

We want to take a screenshot at the end of each selenium test, even if (and especially) when the test fails.
So essentially we need to take the screenshot as part of the test teardown. Here's the relevant section of the pytest docs

https://docs.pytest.org/en/6.2.x/fixture.html#teardown-cleanup-aka-fixture-finalization

Our case is complicated by the fact that we somehow need to pass the screenshot name to the teardown method.
Here's the relevant section in docs and two SO questions were helpful:

https://docs.pytest.org/en/6.2.x/fixture.html#fixtures-can-introspect-the-requesting-test-context
https://stackoverflow.com/questions/58549670/pass-a-parameter-to-a-pytest-fixture-for-teardown

https://stackoverflow.com/questions/38488571/attributeerror-when-using-request-function-in-pytest-yield-fixture

In this PR, we take the approach of having a separate yield fixture. We pass the screenshot filename as a parameter to a dictionary object that is yielded by the fixture. Still feels a bit hacky, but it is quite clean and compact.

I'll also submit an PR with an alternative solution, where we extend the existing selenium_driver fixture.

Closes #419

[unkcpz]

thanks a lot! @danielhollas
This is very interesting, and I prefer this approach as you said having a dedicated fixture is more clear and easy to maintain in the future.

But is that possible to make this fixture call as a function by screenshot("process-list.png")?
If I want to take screenshots at different stages of the test, how can I do it with this fixture?

[unkcpz]

Is the fixture must be called at the very beginning or it can be moved to the last which makes more sense?

[yakutovicha]

I think running this at the beginning is better. This line sets the screenshot filename and puts on hold the rest of the code that follows yield:

aiidalab-widgets-base/tests/conftest.py

Lines 98 to 101 in 33551be

	yield ctx
	if not ctx["name"]:
	raise ValueError("Empty the screenshot filename") # noqa: TC003
	selenium.get_screenshot_as_file(f"{screenshot_dir}/{ctx['name']}")

The code after yield will be executed once all the tests are completed. More information can be found following the link shared by @danielhollas.

[danielhollas]

But is that possible to make this fixture call as a function by screenshot("process-list.png")?
If I want to take screenshots at different stages of the test, how can I do it with this fixture?

The whole purpose of this fixture is to be called at the end of the test, regardless of whether test succeeded or not. So if you need to take screenshot somewhere in the middle, you should still use selenium driver directly as before. Does that make sense?

[unkcpz]

So if you need to take screenshot somewhere in the middle, you should still use selenium driver directly as before. Does that make sense?

It makes sense. That also means with this fixture it is expected to have only one screenshot for a test case. I'd then suggest setting the filename of the screenshot based on the function name, and then the fixture called inside the test function screenshot["name"] = "<test_function_name>.png" can be removed. Is that possible to implement?

[danielhollas]

That's a good idea and I actually did try to implement exactly that, but couldn't make it work. I'll try a bit more. Dne čt 5. 1. 2023 10:03 dop. uživatel Jusong Yu ***@***.***> napsal:

…

So if you need to take screenshot somewhere in the middle, you should still use selenium driver directly as before. Does that make sense? It makes sense. That also means with this fixture it is expected to have only one screenshot for a test case. I'd then suggest setting the filename of the screenshot based on the function name, and then the fixture called inside the test function screenshot["name"] = "<test_function_name>.png" can be removed. Is that possible to implement? — Reply to this email directly, view it on GitHub <#421 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACIY64IGTK7GF3EJ27BSLZTWQ2L7LANCNFSM6AAAAAATAWLULU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[yakutovicha]

Thanks, @danielhollas. I also prefer this approach w.r.t. #422.

I have just a few suggestions.

[danielhollas]

@yakutovicha @unkcpz thanks for the reviews! I've modified the new fixture so that the screenshot name is determined automatically from the test function name. For example, this test

def test_something_important(final_screenshot):
       pass

will automatically generate a screenshot titled something_important.png.

I've also changed the fixture name from screenshot to final_screenshot to make it clear that this is a teardown fixture.

[unkcpz]

@danielhollas thanks! This is very good now. Just have one minor request. I think it is better to add a dummy test that will fail and no screenshot taken but not stop other tests to show it works as expected.

[unkcpz]

Ah, using request.function, didn't know that. This is brilliant!

[unkcpz]

Suggested change

	screenshot_name = f"{request.function.__name__[5:]}.png"
	screenshot_name = f"{request.function.__name__.lstrip('test_')}.png"

Maybe this is more pointy to the purpose?

[danielhollas]

I don't think lstrip here does what you think:

https://stackoverflow.com/questions/4148974/removing-a-prefix-from-a-string

[unkcpz]

I see, then just ignore my request.

[danielhollas]

Note that in Python 3.9 one could use the removeprefix string method for exactly this, but we'd need to drop support for Python 3.8, at least in tests. Probably not worth it now.

[unkcpz]

Looks good to me. @danielhollas I'll let you merge and polish the commit message.

[danielhollas]

I think it is better to add a dummy test that will fail and no screenshot taken but not stop other tests to show it works as expected.

In the last commit I added an intentionally failing test to verify that the fixture works as intended. However, I don't think having this failing test around adds value and it is confusing since it is not testing the application in any way. In the pytest output it now says

============ 10 passed, 1 xfailed, 20 warnings in 183.90s (0:03:03) ============

which again to me rather adds to the confusion.

Leaving up to you. If you want to remove it feel free to revert the last commit.

Looks good to me. @danielhollas I'll let you merge and polish the commit message.

I don't have privileges to merge.