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

memory-package-test-refactor: refactored the memory package test and cleaned up some docblock comments #23

Merged
merged 5 commits into from
Sep 28, 2011
Merged

memory-package-test-refactor: refactored the memory package test and cleaned up some docblock comments #23

merged 5 commits into from
Sep 28, 2011

Conversation

wilmoore
Copy link

  • Corrected a few docblock comments in MemoryPackageTest.php
  • Re-factored MemoryPackageTest making it a bit more dry using data provider and added more fixtures to get a few different looks

End Result:

$ phpunit --testdox --filter MemoryPackageTest
PHPUnit 3.5.15 by Sebastian Bergmann.

Composer\Test\Package\MemoryPackage
[x] Memory Package Has Expected Naming Semantics
[x] Memory Package Has Expected Versioning Semantics
[x] Memory Package Has Expected Marshalling Semantics

…e a bit more DRY and "hopefully" re-usable in anticipation of more package types being adding in the future.
@@ -5,6 +5,7 @@
*
* (c) Nils Adermann <naderman@naderman.de>
* Jordi Boggiano <j.boggiano@seld.be>
* Wil Moore III <wil.moore@wilmoore.com>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should not add your name in the license header but in the @author annotation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also test classes typically don't have author annotations

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough...dropping my name from the header -- not adding @author since it isn't there in any of the other test classes.

@Seldaek
Copy link
Member

Seldaek commented Sep 27, 2011

We use camel case for method names everywhere, please adjust the test methods, and prefix them with test, so you can remove the @test annotation too. Even dox output should be able to cope with testMemoryPackageHasExpectedNamingSemantics.

As for your todo entry, yes, all packages should follow the same semantics so they could be tested together, but we only have the MemoryPackage atm, and I honestly don't know if we'll ever have another package implementation in core.

@wilmoore
Copy link
Author

All updated.

*
* demonstrates several versioning schemes
*/
public function ProviderVersioningSchemes()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should be lowercase too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough.

*
* demonstrates several versioning schemes
*/
public function providerversioningschemes()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to be so anal about all this, but what he meant by lowercase, was only that method names should be camelCased with first letter lower cased, now completely lowercased à la imagecreatefromjpeg(), which is kind of unreadable :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries. That was quickly correctable.

@Seldaek Seldaek merged commit 97de452 into composer:master Sep 28, 2011
@Seldaek
Copy link
Member

Seldaek commented Sep 28, 2011

Merged, thanks

digitalkaoz pushed a commit to digitalkaoz/composer that referenced this pull request Nov 22, 2013
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.

None yet

4 participants