-
Notifications
You must be signed in to change notification settings - Fork 4
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
base: master
Are you sure you want to change the base?
Conversation
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(); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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. 😉
There was a problem hiding this comment.
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']) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I did a very quick review for now. Thanks for your effort. |
Hi @soullivaneuh , I have modified the code and you can review again. I think it's almost done. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this 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 this?
@@ -0,0 +1,6 @@ | |||
{ | |||
"ftpserver": "nexycrypt.96.lt", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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----- |
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.
Change log
Here are the results about code coverage and travis-ci.
code coverage
travis-vi