Rex icon indicating copy to clipboard operation
Rex copied to clipboard

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

Open bbrtj opened this issue 2 years ago • 2 comments

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

Checklist

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

bbrtj avatar Jun 12 '23 20:06 bbrtj

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.

bbrtj avatar Jun 13 '23 15:06 bbrtj

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 :+1: 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.

ferki avatar Jun 16 '23 23:06 ferki