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

Improve excluded files handling in Rex::Commands::Sync #1598

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bbrtj
Copy link

@bbrtj bbrtj commented Jun 12, 2023

This PR is a proposal to fix #1597 by refactoring Rex::Commands::Sync .

Checklist

  • based on top of latest source code
  • changelog entry included
  • automated tests pass
  • git history is clean
  • git commit messages are well-written

@bbrtj
Copy link
Author

bbrtj commented Jun 13, 2023

Windows tests are failing (chmod error) - maybe that's the reason sync was not in automated tests up until now?

Edit: seems to only apply to dir with spaces, so maybe easily fixable nonetheless.

@ferki
Copy link
Member

ferki commented Jun 16, 2023

Windows tests are failing (chmod error)

My first thought was that Windows doesn't have chmod, but apparently Rex:Interface::Base::chmod() should fake success in those cases (and t/file.t should also cover that situation).

maybe that's the reason sync was not in automated tests up until now?

Hmm, the reason is more like Rex::Commands::Sync was a quite early feature, and at the time tests were (sadly) often neglected. It's certainly time to add proper tests now before we are going to change related code

In similar situations I tend to first push only the tests as a first commit to a draft PR (or even only to my own fork) and make sure my tests are correct. Then start on the actual change(s) in separate commit(s). Perhaps it's best to follow something like that here too (or even just focus on an "Add initial sync tests" PR first).

It might also be relevant to keep "When do I use SKIP vs. TODO?" in mind. If something is unsupported or impossible to test on a platform that may be OK to SKIP in the test suite. If something is known to fail, but possible/have to be fixed later, that may be OK to mark as TODO.

Edit: seems to only apply to dir with spaces, so maybe easily fixable nonetheless.

Good catch 👍 Paths with special characters, like spaces, could be requiring special quoting and/or escaping on Windows (and/or other platforms).

ps.: given the test failures, I haven't done a full code review yet, but please consider using explicit test plans overall and in subtests too.

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.

Rex::Commands::Sync excludes are poorly handled
2 participants