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

add unit testing fix some bugs #4

Open
wants to merge 86 commits into
base: master
Choose a base branch
from

Conversation

peter279k
Copy link

@peter279k peter279k commented Oct 3, 2016

Change log

  1. change the steps (1 to 3) to step (0 to 3) because of some bugs.
  2. fix some bugs
  3. write the unit testing and test the main PHP classes. (NexyCrypt and PrivateKey).

Here are the results about code coverage and travis-ci.

code coverage
travis-vi

@peter279k
Copy link
Author

I complete the all unit testing and solve the verify problem. Please review it and give some suggestions. Thanks.

@@ -81,7 +81,7 @@
}
}
} catch (AcmeApiException $e) {
var_dump($e->getDetails());
echo $e->getDetails();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a newline after this?

if ($argc < 3) {
echo 'You have to pass domain and step arguments.'.PHP_EOL;
if ($argc !== 2) {
echo 'You have to pass too many arguments.'.PHP_EOL;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean? This sentence is not understandable.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, you're right . This syntax is not readable so I use > to make syntax easy to understand.

@@ -13,8 +13,7 @@

<filter>
<whitelist>
<file suffix=".php">./src/NexyCrypt.php</file>
<file suffix=".php">./src/PrivateKey.php</file>
<directory suffix=".php">./src/</directory>
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for suffix key.

Please use this file: https://github.com/nexylan/slack-bundle/blob/master/phpunit.xml.dist

We have the same for all Nexylan's projects. 😉

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I remove the suffix attribute.

@@ -320,7 +320,7 @@ private function signedPostRequest($uri, array $payload)
*
* @return ResponseInterface
*/
private function request($method, $uri, array $options = ['verify' => false])
private function request($method, $uri, array $options = ['verify' => __DIR__.'/cacert.pem'])
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is still the same: Passing a array as default argument.

BTW, if you pass a path, the option name should be changed to be more consistent.

Copy link
Author

Choose a reason for hiding this comment

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

I think it's a proper way to solve the SSL certificate problem or what's your suggestion ?
And the which the pem file name is more consistent ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please see: #4 (comment)

And I'm not talking about the filename but the option name.

verify is not very appropriate for a path.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I know it. I will remove the default verify key then it should be fine.

@soullivaneuh
Copy link
Contributor

I did a very quick review for now. Thanks for your effort.

@peter279k
Copy link
Author

Hi @soullivaneuh , I have modified the code and you can review again. I think it's almost done.

Copy link
Contributor

@soullivaneuh soullivaneuh left a comment

Choose a reason for hiding this comment

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

  • What is the goal of cacert.pem?

Sorry for the very long delay.

I'll not merge it right now but will get a closer view when it will be possible.

Thanks for that1 👍

- travis_wait php generate_fake.php 3

script:
- vendor/bin/phpunit
Copy link
Contributor

Choose a reason for hiding this comment

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

Use global PHPUnit instead.


before_script:
- composer require phpunit/phpunit
- travis_wait php generate_fake.php 0
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the goal of this?

@@ -0,0 +1,6 @@
{
"ftpserver": "nexycrypt.96.lt",
Copy link
Contributor

Choose a reason for hiding this comment

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

What is that? A service to create temporary servers?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like a web server just created by you. Am I right?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I just want to do the real testing so I create this FTP server.
But I'm not sure this FTP server is available now.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is. And I'm sure you understand we can't rely on a real external server.

Plus, you should change the credentials, this is not secure at all. 😕

IMO we should just mock the http request. Also see #7

Copy link
Author

Choose a reason for hiding this comment

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

According to this post, perhaps we can let the FTP user name and password being the encrypted environment variables.

What do you think?Or just use the mock to pass this testing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use mock for global testing. For that, you should wait #7 to be merged and use this: https://github.com/php-http/mock-client

At least, we may create a single real test with HTTP challenge using ngrok.

I'll also try to setup simple test on my side for the bridge.

@@ -0,0 +1,15 @@
-----BEGIN RSA PRIVATE KEY-----
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be under a fixtures folder.

@soullivaneuh
Copy link
Contributor

Also, could you please make an interactive rebase to have a clearer git history?

before_script:
- composer require phpunit/phpunit
- travis_wait php generate_fake.php 0
- travis_wait php generate_fake.php 1
Copy link
Contributor

Choose a reason for hiding this comment

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

travis_wait is not necessary here.

@@ -0,0 +1,105 @@
<?php

// generate the fake .well-known folder and upload the folder to the testing web hosting.
Copy link
Contributor

Choose a reason for hiding this comment

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

Generates

$domain = $accounts['ftpserver'];

// First commented line is for production.
//$client = new NexyCrypt();
Copy link
Contributor

Choose a reason for hiding this comment

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

Commented code should not be here. Plus, I don't think this should be used for production.

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