Skip to content

Base Path in renderer #689

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

Closed
wants to merge 6 commits into from
Closed

Base Path in renderer #689

wants to merge 6 commits into from

Conversation

danon
Copy link

@danon danon commented Aug 3, 2019

I found an issue: #610

I really would like to pass my custom basePath for the coverage HTML report. Right now it's generated by AbstractNode#getId() without any form of customization.

This is my first phpunit PR, let me know if it's ok.

@codecov
Copy link

codecov bot commented Aug 3, 2019

Codecov Report

Merging #689 into master will decrease coverage by 0.21%.
The diff coverage is 93.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #689      +/-   ##
============================================
- Coverage     85.05%   84.84%   -0.22%     
- Complexity      813      814       +1     
============================================
  Files            32       32              
  Lines          2476     2481       +5     
============================================
- Hits           2106     2105       -1     
- Misses          370      376       +6
Impacted Files Coverage Δ Complexity Δ
src/Report/Html/Facade.php 86.66% <86.66%> (+0.75%) 14 <14> (ø) ⬇️
src/Report/Html/Renderer.php 97.03% <97.03%> (-1.48%) 29 <29> (+1)
src/Report/Xml/BuildInformation.php 93.93% <0%> (-6.07%) 9% <0%> (ø)
src/CodeCoverage.php 72.48% <0%> (-0.53%) 182% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7743bbc...34937eb. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 3, 2019

Codecov Report

Merging #689 into master will increase coverage by 0.03%.
The diff coverage is 87.17%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #689      +/-   ##
============================================
+ Coverage     85.05%   85.08%   +0.03%     
- Complexity      813      814       +1     
============================================
  Files            32       32              
  Lines          2476     2481       +5     
============================================
+ Hits           2106     2111       +5     
  Misses          370      370
Impacted Files Coverage Δ Complexity Δ
src/Report/Html/Renderer.php 98.51% <100%> (+0.01%) 29 <2> (+1) ⬆️
src/Report/Html/Facade.php 86.66% <86.66%> (+0.75%) 14 <14> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7743bbc...a637613. Read the comment docs.

@sebastianbergmann
Copy link
Owner

Thank you for your contribution. I appreciate the time you invested in preparing this pull request. However, I am reluctant to merge it as I do not understand the problem this is trying to solve.

@danon
Copy link
Author

danon commented Aug 4, 2019

@sebastianbergmann Did you see the issue I linked? There's been a discussion about another attribute basePath in <logging><log>.

For the attribute to have any effect, the Renderer should have a way to put custom base path in the templates, right?

@sebastianbergmann
Copy link
Owner

Yes, I saw the issue linked. I still do not understand what the problem is supposed to be. The generated HTML works just fine, both locally through file:/// as well as served from a webserver, for me.

@danon
Copy link
Author

danon commented Aug 4, 2019

It does work for me as well both directly and through a web server, but only on root page, e.g. localhost/. If i put it in a folder, like localhost/my-coverage-reportit doesn't.

This is because every asset in template links to css and js, and not my-coverage-report/css.

@danon
Copy link
Author

danon commented Aug 4, 2019

Just put your generated report in a nested folder, and you'll see Styles and scripts aren't loaded.

@danon
Copy link
Author

danon commented Aug 4, 2019

This could be either solved by prepending all assets with base path, or by using <base> tag. Each of them requires a change in Renderer.

@sebastianbergmann
Copy link
Owner

I was able to reproduce it, thanks to your explanation.

However, I do not understand why this is happening. The top-level index.html, for instance, has <link href="_css/bootstrap.min.css" rel="stylesheet" type="text/css"> and relative from that top-level directory _css/bootstrap.min.css exists. This works fine when opened through file:/// and when the top-level directory of the code coverage report is hosted by a webserver from its document root. Why does this not work when it is put into a subdirectory of the document root?

This needs to be addresses, I agree, but can we do so without adding another configuration variable?

@theseer
Copy link
Contributor

theseer commented Aug 5, 2019

I cannot reproduce this.

All links in the generated html files are relative - even including ../../../_css where needed when in nested folders. Seems to work just fine.

Running this through nginx or php -S doesn't make a difference:

theseer@nyda /tmp/test-project/build master $ php -S 127.0.0.1:8081 -t .

PHP 7.3.8 Development Server started at Mon Aug  5 10:30:55 2019
Listening on http://127.0.0.1:8081

Document root is /tmp/test-project/build
Press Ctrl-C to quit.

[Mon Aug  5 10:31:17 2019] 127.0.0.1:56396 [200]: /code-coverage/index.html
[Mon Aug  5 10:31:17 2019] 127.0.0.1:56398 [200]: /code-coverage/_css/bootstrap.min.css
[Mon Aug  5 10:31:17 2019] 127.0.0.1:56400 [200]: /code-coverage/_css/octicons.css
[Mon Aug  5 10:31:17 2019] 127.0.0.1:56402 [200]: /code-coverage/_css/style.css
[Mon Aug  5 10:31:17 2019] 127.0.0.1:56404 [200]: /code-coverage/_css/custom.css
[Mon Aug  5 10:31:17 2019] 127.0.0.1:56406 [200]: /code-coverage/_icons/file-directory.svg
[Mon Aug  5 10:31:24 2019] 127.0.0.1:56412 [200]: /code-coverage/content/index.html

Even multiple levels of sub-directories are no issue:

127.0.0.1 - - [05/Aug/2019:10:25:17 +0200] "GET /foo/bar/coverage/_js/nv.d3.min.js HTTP/1.1" 200 217818 "http://localhost/foo/bar/coverage/dashboard.html" "Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0" "-"

What do I have to do to trigger this issue?

@danon
Copy link
Author

danon commented Aug 5, 2019

I cannot reproduce this.

All links in the generated html files are relative - even including ../../../_css where needed when in nested folders. Seems to work just fine.

You're talking about generating a report in a nested folder structure. My problem is generating it in one structure, and then presenting it in another.

What do I have to do to trigger this issue?

Generate the raport, and then move it to another folder or rename the folder it's in (basically, just change the path).

@danon
Copy link
Author

danon commented Aug 5, 2019

This needs to be addresses, I agree, but can we do so without adding another configuration variable?

I will investigate it later today, around 19:00 today and I will give you an update, how to fix it without setting that variable, ok?

@theseer
Copy link
Contributor

theseer commented Aug 5, 2019

I cannot reproduce this.
All links in the generated html files are relative - even including ../../../_css where needed when in nested folders. Seems to work just fine.

You're talking about generating a report in a nested folder structure. My problem is generating it in one structure, and then presenting it in another.

What do I have to do to trigger this issue?

Generate the raport, and then move it to another folder or rename the folder it's in (basically, just change the path).

theseer@nyda /tmp/test-project/build master $ mv code-coverage foo-bar

theseer@nyda /tmp/test-project/build master $ php -S 127.0.0.1:8081 -t .

PHP 7.3.8 Development Server started at Mon Aug  5 11:00:11 2019
Listening on http://127.0.0.1:8081
Document root is /tmp/test-project/build
Press Ctrl-C to quit.

[Mon Aug  5 11:00:28 2019] 127.0.0.1:56596 [200]: /foo-bar/index.html
[Mon Aug  5 11:00:28 2019] 127.0.0.1:56598 [200]: /foo-bar/_css/bootstrap.min.css
[Mon Aug  5 11:00:28 2019] 127.0.0.1:56602 [200]: /foo-bar/_css/style.css
[Mon Aug  5 11:00:28 2019] 127.0.0.1:56600 [200]: /foo-bar/_css/octicons.css
[Mon Aug  5 11:00:28 2019] 127.0.0.1:56604 [200]: /foo-bar/_css/custom.css
[Mon Aug  5 11:00:28 2019] 127.0.0.1:56606 [200]: /foo-bar/_icons/file-directory.svg

Works like charm?
What am I missing? :-)

@theseer
Copy link
Contributor

theseer commented Aug 5, 2019

Again, all links to assets are relative. The actual name of the base directory is irrelevant, so it's not surprising it works.

I do believe you have an issue with it but I'm not convinced it's based on the links.

@theseer
Copy link
Contributor

theseer commented Aug 5, 2019

Okay, looks like it's a path problem after all - but different than one might think.

Trying

theseer@nyda ~/test $ ./tools/phpunit --coverage-html coverage && mkdir /tmp/example && cp -r coverage /tmp/example && cd /tmp && php -S localhost:8080 -t .

When opening http://localhost:8080/example/coverage:

Mon Aug  5 11:50:14 2019] [::1]:34852 [200]: /example/coverage
[Mon Aug  5 11:50:14 2019] [::1]:34854 [404]: /example/_css/bootstrap.min.css - No such file or directory
[Mon Aug  5 11:50:14 2019] [::1]:34856 [404]: /example/_css/octicons.css - No such file or directory
[Mon Aug  5 11:50:14 2019] [::1]:34858 [404]: /example/_css/style.css - No such file or directory
[Mon Aug  5 11:50:14 2019] [::1]:34860 [404]: /example/_css/custom.css - No such file or directory
[Mon Aug  5 11:50:14 2019] [::1]:34862 [404]: /example/_icons/file-directory.svg - No such file or directory

But when opening the actually technically correct http://localhost:8080/example/coverage/:

[Mon Aug  5 12:00:51 2019] [::1]:35020 [200]: /example/coverage/
[Mon Aug  5 12:00:51 2019] [::1]:35022 [200]: /example/coverage/_css/bootstrap.min.css
[Mon Aug  5 12:00:51 2019] [::1]:35024 [200]: /example/coverage/_css/octicons.css
[Mon Aug  5 12:00:51 2019] [::1]:35028 [200]: /example/coverage/_css/custom.css
[Mon Aug  5 12:00:51 2019] [::1]:35026 [200]: /example/coverage/_css/style.css
[Mon Aug  5 12:00:51 2019] [::1]:35030 [200]: /example/coverage/_icons/file-directory.svg

The first URI is technically invalid and shouldn`t even return the directory index.html without redirecting to the second. Maybe that qualifies as a bug in the PHP cli webserver. Nginx for instance does redirect.

@theseer
Copy link
Contributor

theseer commented Aug 5, 2019

Maybe it helps to clarify what I mean with "technically invalid".

The request to /some/dir/foo implies that foo in the directory /some/dir is a file. Logically, when requesting relative URLs like css/bar.css, the file part is stripped from the originating request URL, leading to /some/dir/css/bar.css rather than what might be expected and in our case requrired /some/dir/foo/css/bar.css.

What PHP should probably be doing is what Nginx - and I believe so did apache when i used it last years ago - do as well: If the requested URL resolves to a directory but has no trailing slash, it should send a 301 to the URL with a trailing slash added.

The problem does not occur when using file:// schema, since you have to open the actual file - index.html in this case. That of course is a technically correct URI and thus the relative resolving performed by the browser works as expected.

tl;dr Imho, this is not a bug in php-code-coverage.

@sebastianbergmann
Copy link
Owner

I agree with @theseer.

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.

3 participants