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

Already on GitHub? Sign in to your account

[WIP] Test suite refactoring #465

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants

Refactored the testsuite:

  • Not using __construct() anymore, instead moved logic into setUp().
  • Using configuration through phpunit.xml features ""
  • Removed Windows specific code (com_create_guid()) to make tests run on linux
  • Removed setUpBeforeClass() in several places.

This is still a work in progress.

I couldnt get the tests to run with the current master/dev branches, therefore I cant compare the successfulness of the refactoring. Do all the tests pass on dev/master or are there failures (Unit/Functional).

Contributor

jcookems commented Jun 12, 2012

All of the tests in Unit should pass in dev/master. The Functional tests have some failures, mostly due to tracked issues.

What do I put into the Certificate Path and whats the Service Id for management? I havent set these variables yet in my tests. Is there any docs how to get the tests running with the current testsuite?

Contributor

ogail commented Jun 12, 2012

@beberlei kindly fill out [this document](http://windowsazure.github.com/docs/Contribution License Agreement.pdf) and send it to azurephpsdk@microsoft.com. Thanks :)

Contributor

ogail commented Jun 12, 2012

@beberlei there's a section at the end of the devbox installation guide which explain how to configure the credentials.

@ogail ogail commented on the diff Jun 12, 2012

.gitignore
@@ -6,8 +6,9 @@ build/pdepend
build/phpdox
channel
phpunit.xml
+phpunit.functional.xml
@ogail

ogail Jun 12, 2012

Contributor

Why the functional tests are added to ignore list? Also you've updated the phpunit.functional.xml with the phpunit "" feature

@beberlei

beberlei Jun 12, 2012

Normally you copy the .dist file into the file name without .dist and then fill out the parameters. This line is just to protect users from accidently commiting their credentials to somewhere.

@ogail

ogail Jun 14, 2012

Contributor

Sounds good, I understand what you mean now.

@ogail ogail commented on the diff Jun 12, 2012

WindowsAzure/Table/Internal/MimeReaderWriter.php
@@ -41,6 +41,19 @@
*/
class MimeReaderWriter implements IMimeReaderWriter
{
+ private function getGuid()
@ogail

ogail Jun 12, 2012

Contributor

The change is great! I think it'd be great if you can:

  • Move this method to WindowsAzure/Common/Internal/Utilities.php
  • Add the function to underscored prefix to function name _getGuid()
  • Add a header comment to mention that this is replacement for com_create_guid() because it's windows specific.

If you don't have time i could do these things and I'd appreciate if we can get that from the start.

@beberlei

beberlei Jun 12, 2012

Should i rather move it to internal utilities or name it _getGuid? It would have to be public static on the utilities class.

I will sure add the comment.

@ogail

ogail Jun 14, 2012

Contributor

no then just moving it to utilities class and keep the name as is getGuid

@ogail ogail commented on the diff Jun 12, 2012

phpunit.xml.dist
@@ -20,6 +20,13 @@
<log type="junit" target="build/logs/junit.xml" logIncompleteSkipped="false"/>
</logging>
+ <php>
+ <var name="AZURESDK_STORAGE_ACCOUNT" value="" />
@ogail

ogail Jun 12, 2012

Contributor

I think we can't put windows azure credentials in phpunit.xml.dist. It'll be visible for public because of that we were using getenv. Or I'm missing something here?

@beberlei

beberlei Jun 12, 2012

The value are empty here to show users what variables exist. You can copy the file over to "phpunit.xml" and "phpunit.functional.xml" and then fill out the parameters with your values. That is much more explicit than having ENV Variables (in my opinion)

@ogail

ogail Jun 14, 2012

Contributor

Make sense then I understand the flow. I still have a question, we own a Jenkins CI server and when it's making the full build I inject the environment variables in a pre build task, so how Jenkins will get the credentials for tests when it's running unit tests?

@beberlei

beberlei Jun 15, 2012

Hm, maybe it makes sense to allow both options in this case. Or a different approach would be, that we put in the variables using [env /] into the phpunit.xml.dist and comment the whole block with xml comments, see http://www.phpunit.de/manual/current/en/appendixes.configuration.html#appendixes.configuration.php-ini-constants-variables

That way one could either use the phpunit.xml configuration, or inject using environment variables. This allows both jenkins and "users" to configure the testrunning easily and the commenting of the [env /] variables still serves as a good documentation what to adjust.

Note: used [] instead of html tags, since they get stripped.

@ogail

ogail Jun 15, 2012

Contributor

The problem with the second approach using <env /> that the credentials will be pushed to the repo and we don't that. I think that supporting both solutions would work fine but how can we support both with minimal code changes?

@ogail

ogail Jun 15, 2012

Contributor

FYI, Jenkins pulls the code automatically from the Github repo and starts it's work, we don't do that manually.

@ogail ogail commented on the diff Jun 12, 2012

tests/framework/TestResources.php
@@ -63,22 +63,38 @@ class TestResources
public static function accountName()
{
- return getenv('AZURE_STORAGE_ACCOUNT');
@ogail

ogail Jun 12, 2012

Contributor

I like that change

@ogail ogail commented on the diff Jun 12, 2012

tests/framework/TestResources.php
@@ -127,10 +143,10 @@ public static function listMessagesSample()
$sample['QueueMessage']['TimeNextVisible'] = 'Fri, 09 Oct 2009 23:29:20 GMT';
$sample['QueueMessage']['DequeueCount'] = '1';
$sample['QueueMessage']['MessageText'] = 'PHRlc3Q+dGhpcyBpcyBhIHRlc3QgbWVzc2FnZTwvdGVzdD4=';
-
@ogail

ogail Jun 12, 2012

Contributor

Why there are lot of changes of adding/removing empty line?

@beberlei

beberlei Jun 12, 2012

I removed trailing whitespaces from this file.

@beberlei

beberlei Jun 12, 2012

You can see a diff without whitespaces with ?w=0 at the url.

@ogail

ogail Jun 14, 2012

Contributor

Sounds good, thanks!

Contributor

ogail commented Jun 12, 2012

@beberlei unfortunately I just noticed that I can't look/comment on the pull request until they (azurephpsdk@microsoft.com) receive [this document](http://windowsazure.github.com/docs/Contribution License Agreement.pdf) signed. Sorry for the inconvenience

Contributor

ogail commented Jun 18, 2012

@beberlei do you have a plan to finish the changes?

Yes, but i haven't had time the last days. I will continue to work on this.

@ghost ghost assigned antonba Aug 28, 2012

Contributor

jcookems commented Dec 18, 2012

I took a look at this changeset, and the merge is complicated because @ogail and I have made some of the changes already in other checkins. I'm going to close for now, but I hope that @beberlei will be able to submit an updated PR in the future.

@jcookems jcookems closed this Dec 18, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment