PHPUnit Tests are failing after Squiz.WhiteSpace.FunctionSpacing change [#2890239]

Problem/Motivation

PHPUnit Tests are failing and preventing CI builds from passing in the default branch. The cause is a change in recent versions of PHP_CodeSniffer that adjusted the Sniff for Squiz.WhiteSpace.FunctionSpacing.

Our tests expect a violation of this Squiz.WhiteSpace.FunctionSpacing.Before sniff, which no longer happens.

Proposed resolution

The change to PHP_CodeSniffer fixes false positives where Squiz.WhiteSpace.FunctionSpacing.Before was reported in violation, even though the function had a preceding inline comment. We should adjust our tests to no longer expect this violation.

To avoid this in future, it would be helpful to commit the composer.lock file, so we could ensure Tests were always run against a known, common, supported version of each dependency. This would stop future test failures due to unrelated changes in dependent packages. This is also the recommended approach documented by composer for managing project dependencies.

Remaining tasks

Commit the composer.lock file to the project

Comments

Comment #1

28 June 2017 at 08:08

chOP created an issue. See original summary.

Comment #2

chOP CreditAttribution: chOP at Technocrat commented 28 June 2017 at 08:11

Issue summary:	View changes
Status:	Active	» Needs review

I've opened a pull request with a fix to this issue at

https://github.com/pfrenssen/coder/pull/2

Comment #3

28 June 2017 at 17:59

pfrenssen committed 9233051 on 8.x-2.x authored by chOP

Issue #2890239 by chOP: PHPUnit Tests are failing after Squiz.WhiteSpace...

Comment #4

pfrenssen

Sofia

CreditAttribution: pfrenssen commented 28 June 2017 at 18:00

Status:

Needs review

» Fixed

Thanks!

Comment #5

pfrenssen

Sofia

CreditAttribution: pfrenssen commented 28 June 2017 at 18:04

Note that the composer.lock file should not be committed since this is a library rather than a standalone project that will be deployed in production. If we would lock the dependencies we would not be alerted if any dependency updates introduce problems. If our dependencies were locked any problems would instead be discovered by our users on their projects, which is not very nice :)

Comment #6

chOP CreditAttribution: chOP at Deloitte Digital, Technocrat commented 29 June 2017 at 03:00

Status:

Fixed

» Active

@pfrenssen

I don't think you're right about committing the composer.lock file to a library. The Composer documentation states:

For your library you may commit the composer.lock file if you want to. This can help your team to always test against the same dependency versions. However, this lock file will not have any effect on other projects that depend on it. It only has an effect on the main project.

This is a common misconception about the composer.lock file, and the main reason it seems to be omitted from many libraries. For CI/CD and collaborative development, it can be very beneficial to commit the lock file.

What are your thoughts? Should we start adding it to the project repository? We could omit it from packaged distributions (zip, tarballs, etc) by ensuring we update the .gitattributes, though I don't think it's a problem to leave it in.

Comment #7

chOP CreditAttribution: chOP at Deloitte Digital, Technocrat commented 29 June 2017 at 05:15

I've added another PR with some suggested improvements to prevent future unexpected test failures caused by dependency updates.

For anyone else who's interested in the effect of comitting the composer.lock file to version control, see the following documentation and let us know your thoughts: