Update unittesting.rst, added section "Test report" #9250
Conversation
Wrote this section, when I made this PR qgis/QGIS#58684, for this issue qgis/QGIS#58631.
|
|
||
| When running the unit tests simply using ``make test`` or ``ctest`` three kinds of output are generated. | ||
|
|
||
| A few summary data files are written to ``build/Testing/Temporary``, in particular ``LastTest.log`` and ``LastTestsFailed.log``. These will be overwritten every time you call ``make test`` or `ctest``. |
There was a problem hiding this comment.
When it comes to files path, please use dedicated rst role, I.e
| A few summary data files are written to ``build/Testing/Temporary``, in particular ``LastTest.log`` and ``LastTestsFailed.log``. These will be overwritten every time you call ``make test`` or `ctest``. | |
| A few summary data files are written to :file:`build/Testing/Temporary`, in particular :file:`LastTest.log` and :file:`LastTestsFailed.log`. These will be overwritten every time you call ``make test`` or ``ctest``. |
Some more occurrences below
|
May I request review from a developer? I have no idea if these information are valid and/or worth documenting so would appreciate some expert feedback. |
|
|
||
| A few summary data files are written to ``build/Testing/Temporary``, in particular ``LastTest.log`` and ``LastTestsFailed.log``. These will be overwritten every time you call ``make test`` or `ctest``. | ||
|
|
||
| A bunch of temporary files are saved to ``$TMPDIR``, and if the tests are written as intended, almost all of this is deleted in the teardown process, at the end of each test item. If any of this data is not cleaned up, by default your OS will eventually clean up all files in ``$TMPDIR`` after some time has expired. |
There was a problem hiding this comment.
I don't think this paragraph needs to be included. It's not really relevant to running the tests
There was a problem hiding this comment.
I was surprised to see that all (or almost all) files created during test was deleted during tear down. I don't know if that is the standard way, but I would have expected them to be left in place, so one can inspect the results; at the very least for the failed tests.
So I wrote this paragraph to help the reader understand that he won't be able to find such files; not even for failed tests. (1)
I must admit, that I am not even sure that my assumption above is correct; ie the assumption that even for a failed test the temporary files are deleted during tear down (1). Can you confirm? Is there a rule or guideline that a unit test should delete all files in tear down?
(1): Except for a few special cases where a few relevant files are saved to $QGIS_TEST_REPORT.
There was a problem hiding this comment.
If it's too much hassle to answer these questions now, then don't. I accept your changes :) If I an answer to these questions any time soon, I might suggest that as a separate PR.
There was a problem hiding this comment.
I was surprised to see that all (or almost all) files created during test was deleted during tear down. I don't know if that is the standard way, but I would have expected them to be left in place, so one can inspect the results; at the very least for the failed tests.
I must admit, that I am not even sure that my assumption above is correct; ie the assumption that even for a failed test the temporary files are deleted during tear down (1). Can you confirm? Is there a rule or guideline that a unit test should delete all files in tear down?
There's no particular guideline. I guess it's more that a well designed test would give enough feedback in the test failure itself to diagnose the error.
🤷♂️
There was a problem hiding this comment.
| A bunch of temporary files are saved to ``$TMPDIR``, and if the tests are written as intended, almost all of this is deleted in the teardown process, at the end of each test item. If any of this data is not cleaned up, by default your OS will eventually clean up all files in ``$TMPDIR`` after some time has expired. | |
| A bunch of temporary files are saved to ``$TMPDIR``, your OS will eventually clean up all files in this directory after some time has expired or after reboot (depends on OS). |
|
Apart from the note above, the rest of the content is valuable and should be included! |
|
|
||
| A few summary data files are written to ``build/Testing/Temporary``, in particular ``LastTest.log`` and ``LastTestsFailed.log``. These will be overwritten every time you call ``make test`` or `ctest``. | ||
|
|
||
| A bunch of temporary files are saved to ``$TMPDIR``, and if the tests are written as intended, almost all of this is deleted in the teardown process, at the end of each test item. If any of this data is not cleaned up, by default your OS will eventually clean up all files in ``$TMPDIR`` after some time has expired. |
There was a problem hiding this comment.
| A bunch of temporary files are saved to ``$TMPDIR``, and if the tests are written as intended, almost all of this is deleted in the teardown process, at the end of each test item. If any of this data is not cleaned up, by default your OS will eventually clean up all files in ``$TMPDIR`` after some time has expired. | |
| A bunch of temporary files are saved to ``$TMPDIR``, your OS will eventually clean up all files in this directory after some time has expired or after reboot (depends on OS). |
| Some of the content written to the ``$QGIS_TEST_REPORT`` folder are html pages, and the unit test code will ask the OS to open these pages in the default browser. On some OS, e.g. Ubuntu since v. 21.04, the default browsers are installed as snap apps, which mean they can't access the typical destinations, either ``/tmp`` or ``~/.tmp``. The user will experience a number of tabs open in the browser, but each showing an error message with access denied instead of the actual html. In this case, we recommend that you either change ``$TMPDIR`` or you you define ``$QGIS_TEST_REPORT``, e.g. | ||
| to ``~/qgis_test_report``. |
There was a problem hiding this comment.
| Some of the content written to the ``$QGIS_TEST_REPORT`` folder are html pages, and the unit test code will ask the OS to open these pages in the default browser. On some OS, e.g. Ubuntu since v. 21.04, the default browsers are installed as snap apps, which mean they can't access the typical destinations, either ``/tmp`` or ``~/.tmp``. The user will experience a number of tabs open in the browser, but each showing an error message with access denied instead of the actual html. In this case, we recommend that you either change ``$TMPDIR`` or you you define ``$QGIS_TEST_REPORT``, e.g. | |
| to ``~/qgis_test_report``. | |
| Some of the content written to the ``$QGIS_TEST_REPORT`` folder are html pages, and the unit test code will ask the OS to open these pages in the default browser. On some OS, e.g. Ubuntu since v. 21.04, the default browsers are installed as snap apps, which mean they can't access the typical destinations, either :file:`/tmp` or :file:`~/.tmp`. The user will experience a number of tabs open in the browser, but each showing an error message with access denied instead of the actual html. In this case, we recommend that you at least change the ``$TMPDIR`` env var to a read/write permitted folder (e.g. :file:`~/my_qgis_tmp/`). |
If the default /tmp is access denied then just changing the $QGIS_TEST_REPORT env var will provide html content referencing the access denied folder /tmp. The html content will be half useful ==> this need to be tested!
There was a problem hiding this comment.
If the default /tmp is access denied then just changing the $QGIS_TEST_REPORT env var will provide html content referencing the access denied folder /tmp. The html content will be half useful ==> this need to be tested!
The code PR has already been merged. I have been using it for a few days, and I have not yet noticed any html in $QGIS_TEST_REPORT that referenced anything outside $QGIS_TEST_REPORT.
CORRECTION: All images are copied to $QGIS_TEST_REPORT. And the Javascript comparator, and the img tags use this path. But some of the hyperlinks in the text body point to the original file, outside $QGIS_TEST_REPORT. That can easily be fixed, but it might take a few days before I have time. Screenshot, with doodles: https://imgur.com/a/5HhBALG
|
For the average person, what is the decision here? Thanks |
|
I'm able to do the fix that I said I would. But not in an elegant way. I feel the code needs some refactoring, to avoid duplicate code. But I'm afraid to do that refactoring because my C++ skills are limited and I don't understand all parts of the code. https://github.com/qgis/QGIS/blob/master/src/core/qgsrenderchecker.cpp Also, there is a flag Furthermore there might be other pieces of test code that create html or other output, which contains references to test data that is not part of the report. So in fact I have changed my personal opinion on this. I think it's better to undo the code PR, and thus remove QGIS_TEST_REPORT again. And instead recommend users with snap browsers to do Sorry for the hassle, specially to @nyalldawson after you approved the code PR. What do you prefer I do? |
Added this section, "Test report", while making this PR qgis/QGIS#58684, for this issue qgis/QGIS#58631.