Problem/Motivation
Over in #2642636: Remove static Output in favor of injected OutputInterface we're replacing the home-grown static Output class with an OutputInterface object provided by the console component.
We need to be able to make sure our work there... works.
Proposed resolution
Add functional testing which covers enough output from each command such that we can proceed to refactor with confidence.
Comments
Comment #2
mile23Added branch
2679673-write-output-tests.Remaining:
Comment #4
mile23Very minimal tests for all commands other than the Init section, which requires input and also Docker containers.
Comment #5
MixologicThese look good to me. I added an @group docker for the test that didnt work in an env without docker running, and tweaked a broken thing that phpunit was complaining about (https://3v4l.org/vVgKH), otherwise we have a beginning. I'll have one more sanity check from somebody, and then we can merge this into dev. Almost RTBC?
Comment #6
dawehnerJust look at some of those tests and they look really reasonable.
I have some test failures when I run them locally, when I run them inside the VM, but don't feel blocked on them:
Comment #9
mile23OK, so in IRC we hammered it out a bit and the problem is that the test has a dependency on the configuration setup. At this stage we can't really mock it out easily, so we decided to add testing documentation about how to set up the testing environment.
@jthorson added that the init commands aren't touched by d.o so this might be an adequate functional test for Output refactoring.
Comment #10
MixologicLGTM. I'd say we can roll this into dev. Im not necessarily saying this is enough tests to start ripping out the Output:: stuff, but its probably good enough to prove that what we're removing and replacing it with will work, but it wont reveal if we forget to replace stuff. Then again, global search is our friend there.
Fire at will.
Comment #11
mile23Merged to dev.
Comment #12
jthorson commentedOnly runs properly the first time ... once you run the tests once, config:save creates a 'foof' configset; and config:load asserts that 'foof' doesn't exist.
Yes, this is a dependency on the configuration setup ... but when you start with a clean slate, and do nothing except run the tests twice, we should expect both iterations to pass. :)
Comment #14
jthorson commentedResolved the configuration setup issues for the full set of 'config' tests. Key fixes were to add interaction input for tests where the configuration setup might cause an interactive prompt, and modify the regex to check for both cases (configuration present or not).
Comment #15
mile23They were passing more than once for me, but yah, that'd be an improvement.
Waiting for VM to start up before I review. :-)
Comment #16
mile23Might be useful locally, but maybe not in the repo. We can kill it in #2642636: Remove static Output in favor of injected OutputInterface though.
The rest is good.
Comment #17
jthorson commentedOoops ... yep, that should come out.
That was simply a way to avoid having to type out the individual class names and paths ... i.e. "phpunit --group failing", and moving the annotation from class to class as I worked through them.
Comment #19
MixologicI think a @failing tag is fine in the repo. There are instances where one of us might want to write a test for a known bug, say run a contrib test that doesnt have tests, and commit it, and somebody else could fix that test...
As for the config changes, these will work for now. Eventually we're going to want to isolate those tests from the developers local environment, as in make the .drupalci config directory a configurable option, such that the tests use .drupalci-test. Then we can have distinct tests that exercise different parts of the process - each test would empty the config directory, and perhaps set it into a state with a fixture. So instead of having logic in the test that allows for multiple responses, depending on the state of the .drupalci config dir, we have distinct tests that exercise both of those situations.
But best doesnt need to block better here. Merged into dev and pushed.
Comment #20
MixologicComment #21
jthorson commentedGood logic on the 'failing' tag ... but in this case, it's no longer failing. ;)