RexOps / Rex

Rex, the friendly automation framework
https://www.rexify.org
717 stars 223 forks source link

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

Open bbrtj opened 1 year ago

bbrtj commented 1 year ago

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

Checklist

bbrtj commented 1 year ago

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 commented 1 year ago

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.