Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PhpComposerTest: Upgrade the tests to be compatible with 1.10.1 #2450

Merged
merged 1 commit into from
Apr 1, 2020

Conversation

mnonnenmacher
Copy link
Member

@mnonnenmacher mnonnenmacher commented Apr 1, 2020

Upgrade the PHP Composer tests to be compatible with Composer version
1.10.1. This includes several changes:

  • Upgrade composer.phar to 1.10.1 in all test projects.
  • Upgrade all lockfiles.
  • Fix the "with-replace" test project. Previously the child2 project was
    configured to replace child4:1.0.0 while the project required
    child4:2.0.0 to demonstrate that the project would still install
    child4:2.0.0 because this version is not replaced by child2. This is
    not allowed anymore in the newest composer version, so change child2
    to replace child4:2.0.0.

@sschuberth
Copy link
Member

* Remove composer.phar from projects that do not require it and use the
  version installed on the system for those tests instead.

What determines whether a projects requires a committed composer.phar or not? So far I though a committed composer.phar is needed if the project cannot be built with the globally installed composer version, and as we usually do not know which version is installed (except in our own CI and Docker image), we always need to commit the composer.phar to be on the safe side.

@mnonnenmacher
Copy link
Member Author

What determines whether a projects requires a committed composer.phar or not? So far I though a committed composer.phar is needed if the project cannot be built with the globally installed composer version, and as we usually do not know which version is installed (except in our own CI and Docker image), we always need to commit the composer.phar to be on the safe side.

Two tests use the composer.phar as dependency, I thought for the other tests it's better to test that the code using the globally installed composer works. But I can update the other composer.phars instead if you think that is better.

@sschuberth
Copy link
Member

But I can update the other composer.phars instead if you think that is better.

I don't necessarily think it's better, I just wanted to understand the criteria by which you decided to include composer.phar or not, as that was not clear to me from the commit message. I'll approve and leave it to you.

sschuberth
sschuberth previously approved these changes Apr 1, 2020
@mnonnenmacher
Copy link
Member Author

mnonnenmacher commented Apr 1, 2020

I'll approve and leave it to you.

It turns out the tests fail with the composer version installed on Travis, so I have upgrade the composer.phar in all test projects instead.

@sschuberth
Copy link
Member

I noticed the file modes of composer.phar files now also changed from 100644 → 100755, is this indented?

Upgrade the PHP Composer tests to be compatible with Composer version
1.10.1. This includes several changes:

* Upgrade PHP on Travis from 7.1 to 7.2.
* Upgrade composer.phar to 1.10.1 in all test project.
* Upgrade all lockfiles.
* Fix the "with-replace" test project. Previously the child2 project was
  configured to replace child4:1.0.0 while the project required
  child4:2.0.0 to demonstrate that the project would still install
  child4:2.0.0 because this version is not replaced by child2. This is
  not allowed anymore in the newest composer version, so change child2
  to replace child4:2.0.0.

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@here.com>
@mnonnenmacher
Copy link
Member Author

I noticed the file modes of composer.phar files now also changed from 100644 → 100755, is this indented?

Thanks, fixed.

@mnonnenmacher mnonnenmacher merged commit 2b106cf into master Apr 1, 2020
@mnonnenmacher mnonnenmacher deleted the upgrade-php-composer branch April 1, 2020 15:53
@sschuberth
Copy link
Member

@mnonnenmacher, this now seems to cause PhpComposerTest failures on AppVeyor in newer PRs. I guess it did not occur in this PR because the task output was cached. Could you maybe have a look?

@mnonnenmacher
Copy link
Member Author

@mnonnenmacher, this now seems to cause PhpComposerTest failures on AppVeyor in newer PRs. I guess it did not occur in this PR because the task output was cached. Could you maybe have a look?

I will look into this. But if it is like you describe that means the cache implementation is broken.

@sschuberth
Copy link
Member

But if it is like you describe that means the cache implementation is broken.

Well, in your commit you only changed synthetic test assets for PHP, and none if these are defined as inputs to the analyzer's funTest task. We have the same problem for many other package manager tests, but declaring all of the test projects as input to the funTest task does not seem feasible... what we would need its separate test tasks for each package manager. Then for these defining their test projects as input would make sense.

@sschuberth sschuberth mentioned this pull request Apr 2, 2020
@mnonnenmacher
Copy link
Member Author

Well, in your commit you only changed synthetic test assets for PHP, and none if these are defined as inputs to the analyzer's funTest task. We have the same problem for many other package manager tests, but declaring all of the test projects as input to the funTest task does not seem feasible... what we would need its separate test tasks for each package manager. Then for these defining their test projects as input would make sense.

Correctness is more important than performance for the tests, I think it's ok to run the whole analyzer test set if any test asset is changed. Adding separate test tasks for the package managers sounds like adding a lot of complexity for little benefit, and should not be a requirement for making the test results reliable.

@sschuberth
Copy link
Member

Correctness is more important than performance for the tests

Agreed, but to do it right, you would also need to depend on the binaries of the external tools, like composer, as the task input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants