Skip to content

Fix: Use absolute path for the report #735

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

Merged
merged 12 commits into from
Sep 14, 2023

Conversation

adrien-berchet
Copy link
Contributor

Fixes #732

@BeyondEvil
Copy link
Contributor

Thanks for this PR.

One problem tho, the test passes without the fix.

❯ git branch         
  beyondevil/fix-report-path-issue
* master

❯ git diff  
diff --git a/testing/test_integration.py b/testing/test_integration.py
index 39b46bf..c109456 100644
--- a/testing/test_integration.py
+++ b/testing/test_integration.py
@@ -377,6 +377,21 @@ class TestHTML:
         assert_that(col_name).contains("::setup")
         assert_that(get_log(page)).contains("ValueError")
 
+    def test_chdir(self, pytester):
+        pytester.makepyfile(
+            """
+            import pytest
+            @pytest.fixture
+            def changing_dir(tmp_path, monkeypatch):
+                monkeypatch.chdir(tmp_path)
+                yield tmp_path
+            def test_function(changing_dir):
+                pass
+        """
+        )
+        page = run(pytester)
+        assert_results(page, passed=1)
+
     @pytest.mark.parametrize("title", ["", "Special Report"])
     def test_report_title(self, pytester, title):
         pytester.makepyfile("def test_pass(): pass")

❯ hatch -e test run pytest -k test_chdir
============================================================================================= test session starts =============================================================================================
platform darwin -- Python 3.9.10, pytest-7.2.0.dev634+gfbfd4b500, pluggy-1.0.0
rootdir: /Users/jimbrannlund/dev/pytest/pytest-html
configfile: tox.ini
testpaths: testing
plugins: mock-3.10.0, xdist-3.2.1, rerunfailures-11.1.2, metadata-3.0.0, html-4.0.0rc5.dev1+gbab8e54.d20230411
collected 68 items / 67 deselected / 1 selected                                                                                                                                                               

testing/test_integration.py .                                                                                                                                                                           [100%]

====================================================================================== 1 passed, 67 deselected in 0.87s =======================================================================================

Copy link
Contributor

@BeyondEvil BeyondEvil left a comment

Choose a reason for hiding this comment

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

Please also add an entry to the Changelog. 🙏

@adrien-berchet
Copy link
Contributor Author

Please also add an entry to the Changelog. 🙏

Sure, I added it.
I followed the examples of the previous entries so I thanked myself 😄 But I can change as you wish.

@BeyondEvil
Copy link
Contributor

Please also add an entry to the Changelog. 🙏

Sure, I added it. I followed the examples of the previous entries so I thanked myself 😄 But I can change as you wish.

No, it's perfect! 👍

@BeyondEvil
Copy link
Contributor

Hmm... is as_uri() not Windows safe? INTERNALERROR> FileNotFoundError: [Errno 2] No such file or directory: 'reports\\report.html'

@RonnyPfannschmidt
Copy link
Member

That would be a bummer

@adrien-berchet
Copy link
Contributor Author

Hmm... is as_uri() not Windows safe? INTERNALERROR> FileNotFoundError: [Errno 2] No such file or directory: 'reports\\report.html'

It seems the path was not resolved to an absolute path?

@BeyondEvil
Copy link
Contributor

BeyondEvil commented Sep 13, 2023

Hmm... is as_uri() not Windows safe? INTERNALERROR> FileNotFoundError: [Errno 2] No such file or directory: 'reports\\report.html'

It seems the path was not resolved to an absolute path?

Related to this maybe?

Beware non-existing file on Windows
If the file does not exist, in Python 3.6 to 3.9 on Windows, resolve() does not prepend the current working directory. See issue 38671, fixed in Python 3.10.

https://stackoverflow.com/a/44569249/3845323

Maybe not, as it failed on 3.10 as well...

@adrien-berchet
Copy link
Contributor Author

Hmm... is as_uri() not Windows safe? INTERNALERROR> FileNotFoundError: [Errno 2] No such file or directory: 'reports\\report.html'

It seems the path was not resolved to an absolute path?

Related to this maybe?

Beware non-existing file on Windows
If the file does not exist, in Python 3.6 to 3.9 on Windows, resolve() does not prepend the current working directory. See issue 38671, fixed in Python 3.10.

https://stackoverflow.com/a/44569249/3845323

Maybe not, as it failed on 3.10 as well...

Should we try this solution from stackoverflow: p = Path.cwd() / "file.txt"?

@adrien-berchet
Copy link
Contributor Author

BTW, the absolute() method was only documented and tested in Py311 but the method itself was not changed, so it was already working before. So I think it's still safe to use it, even before Py311.

@BeyondEvil
Copy link
Contributor

This is so classic.... "It's an easy fix" 🤣

@adrien-berchet
Copy link
Contributor Author

The test was not using the Path.as_uri() method to create the expected value so it was not consistent in the Windows case. Now it should be good 🤞

@BeyondEvil
Copy link
Contributor

BeyondEvil commented Sep 13, 2023 via email

@adrien-berchet
Copy link
Contributor Author

I can but I don't think it will work since the job https://github.com/pytest-dev/pytest-html/actions/runs/6169284428/job/16743379351 failed because the resolve() did not prepend the CWD on Windows, as stated in https://stackoverflow.com/questions/42513056/how-to-get-absolute-path-of-a-pathlib-path-object/44569249#44569249 .

@BeyondEvil
Copy link
Contributor

I can but I don't think it will work since the job https://github.com/pytest-dev/pytest-html/actions/runs/6169284428/job/16743379351 failed because the resolve() did not prepend the CWD on Windows, as stated in https://stackoverflow.com/questions/42513056/how-to-get-absolute-path-of-a-pathlib-path-object/44569249#44569249 .

What’s weird is that it failed on 3.10 as well. Where it’s supposed to work.

@adrien-berchet
Copy link
Contributor Author

I can but I don't think it will work since the job https://github.com/pytest-dev/pytest-html/actions/runs/6169284428/job/16743379351 failed because the resolve() did not prepend the CWD on Windows, as stated in https://stackoverflow.com/questions/42513056/how-to-get-absolute-path-of-a-pathlib-path-object/44569249#44569249 .

What’s weird is that it failed on 3.10 as well. Where it’s supposed to work.

True... Let's see what the CI says

@adrien-berchet
Copy link
Contributor Author

Ah now it works for Python>=3.10 and fails as expected for older versions. So I guess we can just revert to Path.cwd() and it will be good?

Copy link
Contributor

@BeyondEvil BeyondEvil left a comment

Choose a reason for hiding this comment

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

Let's release this into the wild and see what happens.

Big thanks for all the work, much appreciated! ❤️

@BeyondEvil BeyondEvil merged commit 82d08c4 into pytest-dev:master Sep 14, 2023
@adrien-berchet adrien-berchet deleted the fix_chdir branch September 14, 2023 07:09
@adrien-berchet
Copy link
Contributor Author

Ok, thank you very much for your help!

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.

Changing dir in test makes pytest-html fail
3 participants