-
Notifications
You must be signed in to change notification settings - Fork 509
Remove parametrized fixtures from pytests for pytest 9 compatability #1417
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
Conversation
|
Were the tests working before? The error message suggests that the parametrization was always ignored, and just now it gives an error. My understanding is that it means the tests behaved before as if that line was just deleted. |
|
I think before the lines were ignored, but the parametrization was always repeated for the actual test function. So i think overall the tests were working and the values of the parameters were passed to the fixtures implicitly. Using the indirect parametrization is the more correct way of doing this, though. |
|
It's possible to keep the test case names using @pytest.mark.parametrize(
"hls_model",
[
("Vivado", "io_parallel"),
("Quartus", "io_parallel"),
("Vivado", "io_stream"),
],
indirect=True,
ids=[
"vivado_parallel",
"quartus_parallel",
"vivado_stream",
],
)
def test_embedding_accuracy(hls_model):
...This will show names like |
|
Cool! I added these ids to all affected tests. |
fastmachinelearning/hls4ml PR fastmachinelearning#1417 fixes
As pointed out by @marco66colombo in #1412, pytest 9 enforces a rule that fixtures can't be parametrized. The way around that is indirect parametrization via the calling test function. This PR makes that change for all tests.
Downsides:
test_embedding_accuracy[Catapult, io_parallel]but rathertest_embedding_accuracy[hls_model3]for example. This will make it more annoying to identify the failing test.Type of change
Tests
Tested all affected test locally in an environment with pytest 9.
Checklist