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.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

Mile23 created an issue. See original summary.

mile23’s picture

Added branch 2679673-write-output-tests.

Remaining:

  • Mock the docker service as per #16 for tests which need docker services.
  • Make at least minimal tests for all commands.

  • Mile23 committed 9b51351 on 2679673-write-output-tests
    Issue #2679673 by Mile23: Create minimal functional tests of command...
mile23’s picture

Status: Active » Needs review

Very minimal tests for all commands other than the Init section, which requires input and also Docker containers.

Mixologic’s picture

These 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?

dawehner’s picture

Just 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:

Time: 1.33 minutes, Memory: 13.75Mb

There were 3 failures:

1) DrupalCI\Tests\Console\Command\Config\ConfigLoadCommandTest::testLoad
Failed asserting that 'You chose configset: /home/vagrant/.drupalci/configs/foof
This will wipe out your current DrupalCI defaults and replace them with the values from the foof configset.
Are you sure you wish to continue? (y/n) Action cancelled.
' matches PCRE pattern "`Unable to load configset. The specified configset does not exist.`".

/home/vagrant/drupalci_testbot/tests/DrupalCI/Tests/Console/Command/Config/ConfigLoadCommandTest.php:20

2) DrupalCI\Tests\Console\Command\Config\ConfigResetCommandTest::testReset
Failed asserting that 'This action will delete the foof configuration set.
Do you wish to continue? (yes/no) ' matches PCRE pattern "`The 'foof' configuration set does not exist.`".

/home/vagrant/drupalci_testbot/tests/DrupalCI/Tests/Console/Command/Config/ConfigResetCommandTest.php:19

3) DrupalCI\Tests\Console\Command\Config\ConfigSaveCommandTest::testSave
Failed asserting that 'The foof config set already exists.
Continuing will overwrite the existing file with the current configuration values.
Are you sure you wish to continue? (yes/no) Action cancelled.
' matches PCRE pattern "`Unable to save an empty configuration set.`".

/home/vagrant/drupalci_testbot/tests/DrupalCI/Tests/Console/Command/Config/ConfigSaveCommandTest.php:19

  • Mixologic committed 74be19f on 2679673-write-output-tests
    Issue #2679673 by Mixologic: adds docker group for things that have to...
  • Mixologic committed 929afb3 on 2679673-write-output-tests
    Issue #2679673 by Mixologic: Fixes a reference passing issue that was...

  • Mile23 committed c54c7dd on 2679673-write-output-tests
    Issue #2679673 Added TESTING.md
    
mile23’s picture

OK, 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.

Mixologic’s picture

Status: Needs review » Reviewed & tested by the community

LGTM. 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.

mile23’s picture

Status: Reviewed & tested by the community » Fixed

Merged to dev.

jthorson’s picture

Status: Fixed » Needs work

Only 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. :)

  • jthorson committed 787f16c on 2679673-write-output-tests
    Issue #2679673 by jthorson: More robust drupalci config::xyz tests
    
jthorson’s picture

Status: Needs work » Needs review

Resolved 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).

mile23’s picture

They were passing more than once for me, but yah, that'd be an improvement.

Waiting for VM to start up before I review. :-)

mile23’s picture

Status: Needs review » Reviewed & tested by the community
+  /**
+   * @group failing
+   */

Might 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.

jthorson’s picture

Ooops ... 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.

  • jthorson committed 2abac88 on 2679673-write-output-tests
    Issue #2679673 by jthorson: Fix docker-rm test assertions
    
Mixologic’s picture

Status: Reviewed & tested by the community » Fixed

I 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.

Mixologic’s picture

jthorson’s picture

Good logic on the 'failing' tag ... but in this case, it's no longer failing. ;)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.