Skip to content

Resolves bug reported in Issue #76 #83

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

Merged
merged 14 commits into from
Apr 12, 2014
Merged

Resolves bug reported in Issue #76 #83

merged 14 commits into from
Apr 12, 2014

Conversation

davidalger
Copy link

Resolves the bug reported in issue #76 and as far as I've been able to verify, doesn't break the copy method in other use cases. If you find I've broken it, let me know and I'll go back to the drawing board… those trailing slashes were being rather pesky. ;)

David Alger added 3 commits March 5, 2014 15:27
@Flyingmana
Copy link
Member

as you can see, you broke the unittests somehow, could you look yourself into the source of the problem?

@davidalger
Copy link
Author

I'm working on resolving those issues. Somehow I didn't notice it had unit tests to run… doh! :/ Anyhow, I'm running them locally to check things now, and may even add a unit test for the big which this is supposed to resolve. ;)

@CNanninga - Tagging you here so you know and will get this thread…

@Flyingmana
Copy link
Member

maybe helpfully, I added some testing with https://github.com/magento-hackathon/magento-composer-installer/blob/master/tests/MagentoHackathon/Composer/Magento/FullStackTest.php to better test problems with real existing modules and created or not created files as result of normal usage.

@davidalger
Copy link
Author

Thanks. I'll check that out and definitely give it a go. There are public OSS modules which are affected by the bug here, but I've got a unit test which I'm testing against for implementation now. :)

@davidalger
Copy link
Author

@Flyingmana Question: How can I get the artifact needed to successfully run the full stack tests? If I merge master into this branch and then run phpunit I get the following output:

There was 1 failure:

1) MagentoHackathon\Composer\Magento\FullStackTest::testFirstInstall
Failed asserting that file "/Volumes/Server/proj/magento-composer-installer/tests/FullStackTest/artifact/magento-hackathon-magento-composer-installer-999.0.0.zip" exists.

/Volumes/Server/proj/magento-composer-installer/tests/MagentoHackathon/Composer/Magento/FullStackTest.php:109
phar:///usr/local/Cellar/phpunit/3.7.28/libexec/phpunit-3.7.28.phar/phpunit/TextUI/Command.php:176
phar:///usr/local/Cellar/phpunit/3.7.28/libexec/phpunit-3.7.28.phar/phpunit/TextUI/Command.php:129

FAILURES!                                             
Tests: 101, Assertions: 184, Failures: 1, Skipped: 11.

Currently branch issue-76 contains all of tag 2.0.0-beta1 and the unit tests run perfectly on my dev setup. Still waiting to hear back from Travis. :)

@Flyingmana
Copy link
Member

that could be probably caused from a missing composer.phar in the path, I use composers archive command to generate the artifact

@Flyingmana
Copy link
Member

could you do a merge, to include the latest unittests?

@davidalger
Copy link
Author

The latest code in master has been merged in.

Just verified that composer.phar is in my path and executes. What is strange is there are a few other artifacts left behind after the error about magento-hackathon-magento-composer-installer-999.0.0.zip not existing. Any other ideas? I just updated composer to make sure it wasn't due to being on a slightly out of date version, but that didn't affect anything.

@davidalger
Copy link
Author

@Flyingmana Do you want me to fix the broken full stack test for this or are you planning on doing that?

@Flyingmana
Copy link
Member

I need a bit of time to think about this and look again trough it, but if you fix it, I would merge it and wait if someone complains about the behavior.

Flyingmana added a commit that referenced this pull request Apr 12, 2014
Resolves bug reported in Issue #76
does not break any existing unittests, currently failing unittests were already failing before
@Flyingmana Flyingmana merged commit b84b310 into magento-hackathon:master Apr 12, 2014
@davidalger davidalger deleted the issue-76 branch April 24, 2014 14:56
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.

3 participants