Skip to content

Navigation Menu

Sign in
Sign up

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

Open
bbrtj wants to merge 2 commits into
RexOps:master from
bbrtj:master
Open

Improve excluded files handling in Rex::Commands::Sync #1598
bbrtj wants to merge 2 commits into
RexOps:master from
bbrtj:master

Conversation

@bbrtj

@bbrtj bbrtj commented Jun 12, 2023
edited
Loading

Copy link
Copy Markdown

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 commented Jun 13, 2023
edited
Loading

Copy link
Copy Markdown
Author

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 Jun 16, 2023

Copy link
Copy Markdown
Member

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rex::Commands::Sync excludes are poorly handled

2 participants

AltStyle によって変換されたページ (->オリジナル) /