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

Code coverage and Yii2 #1024

Closed
nkovacs opened this issue May 16, 2014 · 28 comments
Closed

Code coverage and Yii2 #1024

nkovacs opened this issue May 16, 2014 · 28 comments
Milestone

Comments

@nkovacs
Copy link
Contributor

@nkovacs nkovacs commented May 16, 2014

I'm using the Yii2 basic app template.

PhpBrowser's url property is set to http://localhost:8080 in acceptance.suite.yml, and then in _bootstrap.php, $_SERVER['SCRIPT_NAME'] is set to '/basic/web/index-test.php'. This is required to make Yii2's url manager work.

The problem is that CodeCoverage::getRemoteCoverageFile takes PhpBrowser's url property, and then appends /c3/report/... to it, which won't work in this case. I assume I'd have to set url to http://localhost:8080/basic/web/index-test.php, but then Yii will not work, and in the example in the documentation, the url is set to http://localhost as well.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented May 16, 2014

i was not using code coverage on UI tests, however you can use in your tests full url with basic/web/index-test.php just make sure that basic is under web root directory of your server.

@nkovacs
Copy link
Contributor Author

@nkovacs nkovacs commented May 19, 2014

No, I can't use the full url in PhpBrowser's config (I'm assuming that's what you meant), because Yii's url manager will generate urls like /basic/web/index-test.php/site/about, and then if you try to pass that url to $I->amOnPage, it'll try to open http://localhost/basic/web/index-test.php/basic/web/index-test.php/site/about, which of course doesn't work.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented May 19, 2014

you are free to edit phpbrowser config in suite yaml.

@nkovacs
Copy link
Contributor Author

@nkovacs nkovacs commented May 19, 2014

I know that, that's what I have been doing, but no matter how I configure it, I cannot get both code coverage and acceptance tests to work.
For code coverage to work, I have to set the url to http://localhost:8080/basic/web/index-test.php, because it does a simple concatenation and adds /c3/report/... to that string.
For acceptance tests to work, I have to set the url to http://localhost because Yii's url manager generates urls that include /basic/web/index-test.php, becase that's the correct url, and PhpBrowser's amOnPage method concatenates that to the url config string.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented May 19, 2014

you should edit settings in _bootstrap.php to set correct url.

@nkovacs
Copy link
Contributor Author

@nkovacs nkovacs commented May 19, 2014

And what's the correct url?

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented May 19, 2014

if you set in yaml config full url, then in _bootstrap.php it will be ''.

@nkovacs
Copy link
Contributor Author

@nkovacs nkovacs commented May 19, 2014

I've tried that already, it doesn't work. Yii expects basename($_SERVER['SCRIPT_NAME']) === basename($_SERVER['SCRIPT_FILENAME']), and throws an exception if it doesn't.

If I set PhpBrowser url to http://localhost/basic/web/ and set TEST_ENTRY_URL to /index-test.php and set up an apache rewrite rule, it works, but that's an ugly hack.

@nkovacs
Copy link
Contributor Author

@nkovacs nkovacs commented May 19, 2014

Scratch that, coverage doesn't work using the last method.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented May 19, 2014

i am not sure about the correct solution since i was not using code coverage for UI tests, will check this one later.

@nkovacs
Copy link
Contributor Author

@nkovacs nkovacs commented May 19, 2014

Well, I added another option to coverage to explicitly set the c3 base url: nkovacs@11f2152
but it's not exactly the most elegant solution.
The problem I think is that PhpBrowser expects url to be the entry point of your app, but it's using it more like a base uri or a hostname, and Yii uses it like that too, because that's how its url manager works.

@nkovacs
Copy link
Contributor Author

@nkovacs nkovacs commented Jul 31, 2014

Bump

@splinter89
Copy link
Contributor

@splinter89 splinter89 commented Jul 31, 2014

You can use special headers like C3-Header: html or C3-Header: clear instead of appending /c3/report/html to URI (which I think is awful idea).
It would be cool if you would upgrade c3 not to touch any URI of app 😊

@nkovacs
Copy link
Contributor Author

@nkovacs nkovacs commented Jul 31, 2014

You still need to be able to specify a base url for c3, otherwise it'll just try to open localhost.

@DavertMik DavertMik added yii labels Aug 2, 2014
@samdark samdark modified the milestones: 2.0.5, 2.0.6 Aug 22, 2014
@DavertMik DavertMik modified the milestones: 2.0.7, 2.0.6, 2.0.8 Oct 5, 2014
@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Nov 9, 2014

@nkovacs is there any PR by you on this issue ? Have same issue and came to same PR solution , so have you submitted it already or just commit in your local Codeception repo ? Thanks

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Nov 9, 2014

@DavertMik can we introduce new option for coverage that will point to url here, like this:

coverage:
    remote_url: 'http://127.0.0.1:8000/index-test.php/'

It will not break BC since if it is not specified then old way will be used, it is currently needed due some Yii2 limitations (maybe will reconsider them later), so nothing should be wrong . Note that i am talking not about module reconfiguration , but about coverage param

@nkovacs
Copy link
Contributor Author

@nkovacs nkovacs commented Nov 9, 2014

I think there was a PR, but I can't find it now, maybe it got deleted when I rebased my branch.

@nkovacs
Copy link
Contributor Author

@nkovacs nkovacs commented Nov 9, 2014

Why do you have to include the hostname in remote_url in your example?

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Nov 9, 2014

Ok , then if it will be ok with @DavertMik i will submit new one , a little bit changed not as yours solution (will also mention you of course)

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Nov 9, 2014

Why do you have to include the hostname in remote_url in your example?

solution with concatenation is not good, it is more like a hack , while ability to specify full url or relative if needed gives you more control on what url it should be , without depending on module that is currently used for interactions with application

@nkovacs
Copy link
Contributor Author

@nkovacs nkovacs commented Nov 9, 2014

But when will the hostname be different? And if you specify a relative url, won't you have to do a concatenation?

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Nov 9, 2014

But when will the hostname be different? And if you specify a relative url, won't you have to do a concatenation?

In different situations, you can have some pre or post processing that can be used to retrieve info. Not sure about relative url, currently will be implementing only full url support

@nkovacs
Copy link
Contributor Author

@nkovacs nkovacs commented Nov 9, 2014

Most of the time the hostname will be the same. Having to repeat it in the config is inconvenient and a source of error.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Nov 9, 2014

I dont think it will be a source of error , while concatenation is only made because of Yii2 path detection for baseUrl, so i think it is fine

@nkovacs
Copy link
Contributor Author

@nkovacs nkovacs commented Nov 9, 2014

If you change phpbrowser url and forget to change coverage remote_url, then it won't work.
The concatenation is necessary because it's a relative url.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Nov 9, 2014

Not really, it is not relative , it is the same url, your concatenation only made as was said because of Yii2 issues, url is the same {your host}/index-test.php, but for some simplicity we use not full url (without index-test.php) for Yii2 baseUrl path detection, so you are not correct that it is relative, it can be the same or completly different, so we should specify it as full url but with the same full value as for other in config.

f you change phpbrowser url and forget to change coverage remote_url, then it won't work.

Well, it is developers issue to check that everything is correct, if you will delete your index file it also will not work.

As a workaround for your solution we can add simple check for http so if it is not presents then it is a relative url

@DavertMik
Copy link
Member

@DavertMik DavertMik commented Nov 9, 2014

I think @Ragazzo proposes the most clean solution. I'd change configuration to:

coverage:
    # url of file which includes c3 router.
    c3_url: 'http://127.0.0.1:8000/index-test.php/'

This should solve the issue. Yet it may be mentioned somewhere, that this option was added specially for Yii. Any ideas where to put this info? Codeception guides, Yii guides?

It looks like this option can provide to have standalone router just with c3 included.
Yet, c3 still should be included into the index of application in order to collect coverage data.
For transfering coverage data user may create another index file with only c3 in it.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Nov 10, 2014

I think it is better to mention it in Codeception guide, i will handle Yii2 guide . Also please note in Codeception guide about remote_config since it is useful feature for remote code coverage and was not mentioned in docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.