The problem is that there might be random test failures, or expected test failures from existing changes etc, especially working in branches etc and then it causes more work most of the time than copying to expected directly and it is easy and quick to compare screenshot tests in phpstorm and copy them over to expected from processed but takes a while to look at local history etc to know what changes you are going to make. etc....

As long as it is possible for a developer to change the default, or pass an option it might be ok to make it default. But there is some "risk" that it makes working with things slower and that you accidentally commit broken screens and then forget to fix the test etc.

The command "only" syncs the system test files. UI files are synced with another command, but are synced to expected folders only.

I see. In this case I would vote quite strongly against this. For system tests it is way too easy to miss some changes in travis and it'd be much safer to first double check all the individual changes in the diff before moving them to expected. For people that might use a visual git commit tool like in PHPStorm that might not be too bad, but for the rest of the world it becomes unusable :)

I'm fine with closing this again. For me I'm always syncing to expected and check the diff with git or simply add what I consider valid with git add -p

Yeah for me it's just not really practical to validate a big diff through git diff

How are you validating the changes then?

PHPStorm

I also use phpstorm to compare the "Expected" folder in the current local branch, to the "expected" on the remote, the result I think is the same. maybe we can make it the default to copy to expected/, but leave the option to copy to the processed/ folder instead

I also use phpstorm to compare the "Expected" folder in the current local branch, to the "expected" on the remote, the result I think is the same.

I don't do that. I compare processed with expected.

I don't use any of the Git features in PHPStorm as it is slow and not transparent what it does etc. So if expected is default, it becomes kind of useless. If there's an option to use processed that's fine. I'll just have to always set the option.

In general I would still prefer the processed to be the default as it reduces the risk that someone just syncs the tests and pushes it and we start expecting wrong results. Happens so easy...

Closing this now. For me it doesn't really make a difference if processed or expected is the default as long as there is a parameter to change the behavior. So let's keep it as is for now.