Skip to content

Conversation

@divyaajainn
Copy link
Contributor

Purpose of PR?: Updates the abstract base class signature of _determine_filename to include a default reload parameter, aligning it with its implementations and usage in have_data.

Bug fix
Fixes #2745

@ReimarBauer
Copy link
Member

see the 2nd task in the issue

Copy link
Member

@ReimarBauer ReimarBauer left a comment

Choose a reason for hiding this comment

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

see comment

@divyaajainn
Copy link
Contributor Author

i will look into it today itself. i was busy with an event.

@divyaajainn divyaajainn requested a review from ReimarBauer April 4, 2025 14:48
@ReimarBauer ReimarBauer requested a review from matrss April 9, 2025 13:26
@divyaajainn
Copy link
Contributor Author

Does this look good?

Copy link
Member

@ReimarBauer ReimarBauer left a comment

Choose a reason for hiding this comment

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

see hint

@divyaajainn
Copy link
Contributor Author

I was going for that initially, but didn’t import pytest at the time so I just quickly checked the behavior with a try-except. I have updated it, does this look good?

@divyaajainn divyaajainn requested a review from ReimarBauer April 10, 2025 10:55
@divyaajainn
Copy link
Contributor Author

I think the error is caused when I clear _filetree (by setting it to {}), the expected ValueError isn’t raised maybe DUT’s setup or a caching mechanism might be preventing it from being empty.
i am thinking of adding an assertion to verify the internal state before calling _determine_filename and use debug logging to see if the except clause is reached.
That way if the implementation needs to be adjusted.

@matrss
Copy link
Collaborator

matrss commented Apr 22, 2025

I think the error is caused when I clear _filetree (by setting it to {}), the expected ValueError isn’t raised maybe DUT’s setup or a caching mechanism might be preventing it from being empty.

The test failure you are seeing is not caused by the first with pytest.raises(ValueError):, but by the second one. Congratulations, you have found a new bug: _determine_filename returns an invalid/outdated filename with reload=True because it blindly trusts the cached filenames in _filetree. @ReimarBauer I think this needs some clarification on how this method is supposed to work, the reload parameter suggests there have been performance concerns in the past that prevented an implementation that tries to always be consistent with the filesystem?

In an attempted fix of this bug I noticed that just appending .bak to a filename in the test is not enough, as the next call to .setup will then pick up the variable from the .bak file.

@divyaajainn
Copy link
Contributor Author

Thanks. I hadn’t realized the failure was from the second ValueError. I’ll hold off on changes for now and wait for clarification on the expected behavior of reload. I’ll also update the test to avoid .bak files being picked up by .setup.

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