Skip to content

Conversation

@ramchale
Copy link
Contributor

  • Only remove temp directory after all tests have run
  • Use DataProvider for filter config and input variations
Q A
Documentation no
Bugfix no
BC Break no
New Feature no
RFC no
QA yes

Description

Draft PR to discuss refactoring of RenameTest.

Started looking at this as part of the moving the Rename filter to v3, but this feels like a big enough piece of refactoring in itself to be worth sense checking.

As DataProviders run before any of the test hooks, the creation of the tmp test file is moved to a static method that creates it on first use, and then it's not removed until all the tests in RenameTest have run. Likewise for the sub directory needed for testing moving a file to a target directory.

The cleanup for the indvidual files created by tests has been moved to each test. Could potentially be moved back to tearDown if we're not concerned about the number of redundant calls, and depending on what seems more readable.

Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

Thanks @ramchale
This test was a real mess huh? LGTM, other than one item.
Hopefully we've got enough in the current 2.x release branch to tag and merge this up into 3.x when its ready 👍

@gsteel gsteel added this to the 2.40.0 milestone Jan 3, 2025
@gsteel gsteel self-assigned this Jan 3, 2025
- Only remove temp directory after all tests have run
- Use DataProvider for filter config and input variations

Signed-off-by: ramchale <legionsofbob@gmail.com>
@ramchale ramchale marked this pull request as ready for review January 6, 2025 12:05
@ramchale
Copy link
Contributor Author

ramchale commented Jan 6, 2025

Thanks @ramchale This test was a real mess huh? LGTM, other than one item. Hopefully we've got enough in the current 2.x release branch to tag and merge this up into 3.x when its ready 👍

Happy New Year!
Unit testing anything involving the file system is always a pain 😓
Thanks. That should be updated now

Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

Thank you @ramchale :)

@gsteel gsteel merged commit 1a4194e into laminas:2.40.x Jan 6, 2025
14 checks passed
@ramchale ramchale deleted the RenameTest branch March 26, 2025 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants