Skip to content

Conversation

RyanNerd
Copy link

Test case for the curl_multi_info_read() function.
Tests required parameters only.

--FILE--
<?php

//CREATE RESOURCES
Copy link
Contributor

Choose a reason for hiding this comment

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

useless comment

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@staabm staabm May 25, 2016

Choose a reason for hiding this comment

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

you should add a comment, when someone reading the code needs information about things not already obvious from the code.

writing something like

// this line will init curl
$c = curl_init();

does not provide more information.

@marcosptf
Copy link
Contributor

marcosptf commented May 25, 2016

dear[]s,

i already did this test since mar/13 on this PR ===> #1809
and the travis CI is running there!!!

@RyanNerd @staabm @rquadling @dsp @reeze

Merci

@RyanNerd
Copy link
Author

http://gcov.php.net/viewer.php?version=PHP_HEAD&func=tested_functions

This indicates that this function as not having coverage. This is the QA
link I was told to use to determine where coverage is missing. Is this the
wrong URL?

On Wed, May 25, 2016 at 6:21 AM, marcosptf notifications@github.com wrote:

dear[]s,

i already did this this test since mar/13 on this PR ===> #1809
#1809
and the travis CI is running there!!!

@RyanNerd https://github.com/RyanNerd @staabm
https://github.com/staabm @rquadling https://github.com/rquadling @dsp
https://github.com/dsp @reeze https://github.com/reeze

Merci


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1922 (comment)

@marcosptf
Copy link
Contributor

this is the official link, but need to see if don't have another PR open to there function, to don't duplicate test.

@RyanNerd
Copy link
Author

Is there a case test I could help with that you can suggest, or should I go
through the official link looking for items that may or may not have
existing PRs?

On Wed, May 25, 2016 at 10:03 AM, marcosptf notifications@github.com
wrote:

this is the official link, but need to see if don't have another PR open
to there function, to don't duplicate test.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1922 (comment)

@marcosptf
Copy link
Contributor

@RyanNerd you are doing very well, just see what the Travis is boring and lets go with this test!

👍

@rquadling
Copy link
Contributor

Any reason why I was included in #1922 (comment)? Maybe someone else should have been included.

@cmb69
Copy link
Member

cmb69 commented Jun 16, 2016

I'd prefer this test over PR #1809, mainly because it's testing against a local server and as such doesn't depend on an external website, and because the expectations are more meaningful.

The failing Travis check appears to be unrelated to this PR.

@marcosptf
Copy link
Contributor

@cmb69 i wish Y! never die!

cross fingers

:-)

@RyanNerd RyanNerd closed this Aug 24, 2016
@RyanNerd RyanNerd reopened this Aug 24, 2016
@RyanNerd
Copy link
Author

Status on this PR?

@cmb69
Copy link
Member

cmb69 commented Aug 24, 2016

Status on this PR?

Two issues:

  • the .phpt should be moved to ext/curl/tests (generally, each test should be put into the appropriate tests/ folder)
  • the test should have a --SKIPIF-- section to prevent the test failing if the curl extension is not available (see how that's done in the other ext/curl/tests/*.phpt)

Otherwise the test is good to merge (I'd prefer to have it in PHP-7.0 also).

@@ -0,0 +1,58 @@
--TEST--
Test function curl_multi_info_read() by calling it with its required argument
--CREDITS--
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add SKIPIF here - look at other CURL tests as an example. Also, please move to ext/curl/tests/

@krakjoe
Copy link
Member

krakjoe commented Jan 7, 2017

PR #1809 was merged, so closing this one.

@krakjoe krakjoe closed this Jan 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants