obspy / reporter

ObsPy test reporter app
3 stars 5 forks source link

support json report uploads #22

Closed d-chambers closed 2 years ago

d-chambers commented 2 years ago

Opening this issue in relation to a discussion in https://github.com/obspy/obspy/pull/2489.

ObsPy's tests have been refactored to use pytest so some modification to the reporter app is needed to support a more pytest friendly output.

Currently I am just using pytest-json-report as it seems to be the best supported json report writer. We could try to stick with xml but it is 2022 after all :wink:.

I have also attached an example json report. It is really quite simple to modify so we can easily make changes. report.zip

@barsch , @Trichter

barsch commented 2 years ago

alright, looking not bad - just some small issues:

(1) missing node name in the JSON document which could be either set with the --node parameter on obspy-runtests or had been autogenerated via https://github.com/obspy/obspy/blob/master/obspy/scripts/runtests.py#L301-L305

Until now this has been extracted from the XML report under /report/platform/node in the XML document, e.g. gh-actions in https://tests.obspy.org/115753/xml/

(2) number of tested modules - obspy-runtests sends this as header parameter - IMHO not needed any more. However, I want to extract that number somehow from the JSON document. Could you create a report with only some selected modules as if I would run obspy-runtests io.wav io.sac - so I can see how to retrieve this within the JSON document ...

barsch commented 2 years ago

(3) install_log missing, see "https://github.com/obspy/obspy/blob/master/obspy/scripts/runtests.py#L642-L643" - don't know if still needed

d-chambers commented 2 years ago

Could you create a report with only some selected modules

Sure, I used obspy-runtests io/wav io/sac --json-report --json-report-indent=2 to generate this file: sac_wav_report.zip

(1) missing node name

Ok, I will add 'node' to the plaform_info namespace. I am a little confused at why the second split is needed though on macos, eg here: https://github.com/obspy/obspy/blob/dab888c72c3644172ce767a636ed83ac9c6a397f/obspy/scripts/runtests.py#L300-L305

Since when we originally define the HOSTNAME we already applied a split on dot here: HOSTNAME = platform.node().split('.', 1)[0]

Also, I want to keep the new obspy-runtests api as concise as possible so I don't want to add another command line argument to control this. Since we just run on github CI right now, I will either set it to github_actions, use an environmental variable OBSPY_NODE_NAME (on the off chance we run in another environment) or just use the default.

install_log missing - don't know if still needed

I am going to guess no. The log is not generated by default so I doubt many (any) users would generate it on their own and attach it. For CI we have access to all the logs through the github actions interface. However, I will add a 'log` top-level entry (null by default) since the report already supports it and it will allow us to use it in the future without any additional changes to the reporter.

I just pushed the updates.

barsch commented 2 years ago

@d-chambers can you please add a few more test reports with multiple errors, so I can work on the traceback formating - sorry don't have obspy locally installed and its probably way faster for you creating a report than me installing the correct branch ...

Also we used to distinct between failures (failing network test) and errors (all other tests) - in the XML they were separate entries (see e.g. http://tests.obspy.org/109726/xml/) and were also visually displayed differently in the HTML output (see http://tests.obspy.org/111826/ - yellow == network tests, red == all others). Unfortunately with the new JSON format there is ofc no indicator which module/tests are considered a network test and which not - can this be included? Original idea was that failing network tests are not that important than not network tests - therefore different visual indicators ...

Another issue is that when modules are skipped there is no more indication which modules are missing - basically the table in this http://tests.obspy.org/115744/ would only contain the obspy.io.sh row and all not installed rows would be missing as the reporter does not know about it. I don't think its an issue and can live with it - but if people are used to it than the JSON needs some information which modules are skipped.

Concerning hostname and MacOS - I assume this is legacy code and can be cleaned up - but someone with MacOS should test ...

d-chambers commented 2 years ago

Sure no problem, will work on it later today

d-chambers commented 2 years ago

I created the following python file for generating simple reports:

"""
A temporary test file for generating reports.
"""
import warnings

import pytest

class TestJsonReport:
    """Simple class for testing json report."""
    @pytest.fixture
    def bad_fixture(self):
        """A fixture which raises an Error."""
        raise ValueError('Text of a ValueError raised in fixture.')

    def test_exception_raised_error(self):
        """A test to see what happens in report when an error is raised."""
        raise ValueError('Text of a ValueError in test code.')

    def test_good_test_bad_fixture(self, bad_fixture):
        """Just use the bad fixture."""
        assert 1

    def test_image_marker(self, image_path):
        """A test which uses the image_path and should have image marker."""

    @pytest.mark.network
    def test_network_marker(self):
        """A test with the network marker."""

    def test_print_something_to_std_out(self):
        """A test for printing."""
        print('hey Robert')

    def test_warning(self):
        """A test for issuing a warning."""
        warnings.warn('Never run with scissors')

    def test_passes(self):
        """Just a nice clean pass test."""
        assert 1

    def test_clean_fail(self):
        """A test which fails via an assert."""
        assert 0

And here is the json report: simple_report.zip

A few things to note:

  1. Any exception raised in the test code itself is reported as a failure on the 'outcome' field of the test entity
  2. Any exception in the fixtures (setup/teardown) reports an error
  3. The whole traceback text is found in the 'longrepr` field for each test stage.

Unfortunately with the new JSON format there is ofc no indicator which module/tests are considered a network test

It is a bit strange but the pytest marks are buried in the keywords field of the test entity. So, on the test level you can check if a test is a network test via:

'network' in test.get('keywords', {})

Another issue is that when modules are skipped there is no more indication which modules are missing

If you select certain obspy modules like so:

obspy-runtests io/sac

pytest only looks in obspy/sac for tests to collect. It doesn't know anything about tests higher up in the directory tree so there is nothing we can do here.

However, if you select marked tests like one of these:

obspy-runtests --network  # run only network tests
obspy-runtests -m image  # run only image tests

Then all the tests should show up in the collectors field on the top level and skipped nodes will indicate they were deselected.

To me it seems like a non-issue though; if it doesn't show up in table it must have been skipped. Pytest is mature enough we don't need to worry too much about its test collection going awry.

EDIT:

There was one error in the report I linked above in that I accidentally left in a hook which added some metadata to the test entities called markers, but this shouldn't be there. The difference is subtle, but here is the correct version: simple_report.zip

trichter commented 2 years ago

@d-chambers I think you should deinstall pytest-metadata again. It is not required at the moment and inserts itself metadata in the JSON. We should also report versions of non-required dependencies (i.e. cartopy and co) and report if these are installed. Different sets of installed packages often lead to unexpected failures in the past.

Since we just run on github CI right now, I will either set it to github_actions, use an environmental variable OBSPY_NODE_NAME (on the off chance we run in another environment) or just use the default.

Just set it to github-actions when running in CI is enough, I think.

Another issue is that when modules are skipped there is no more indication which modules are missing To me it seems like a non-issue though; if it doesn't show up in table it must have been skipped. Pytest is mature enough we don't need to worry too much about its test collection going awry.

We could generate the aggregations (slowest tests and number of running tests, number of fails per module) directly in obspy/conftest.py. With the output from json-reporter this easy to do compared to the JunitXML output. Missing modules then simply show up with 0 tests.

d-chambers commented 2 years ago

I think you should deinstall pytest-metadata again. It is not required at the moment and inserts itself metadata in the JSON.

This is a losing battle because pytest-metadata is required by pytest-json-report.

We could generate the aggregations (slowest tests and number of running tests, number of fails per module) directly in obspy/conftest.py. With the output from json-reporter this easy to do compared to the JunitXML output. Missing modules then simply show up with 0 tests.

The slowest tests should be easy to get on the server side since the run times are already in the Test entities. However, I am not sure how to generate the aggregations when only sub modules are selected. Do you have an idea how to do it?

Also, would it be worth the slowdown? For example, right now obspy-runtests io/sac runs in under one second on my machine. For the whole test suite it takes about 8 seconds just to collect the tests, so it would be a 10x slowdown. Maybe not a big deal, but it could be annoying if you are just trying to run one test. I guess we wouldn't do it unless the report option is selected so it is maybe not an issue.

barsch commented 2 years ago

Alright, reporter app has been updated and it will accept now the new JSON format - see https://tests.obspy.org/115755/ for a test report

JSON documents need to be sent to a new endpoint /post/v2/ and can be uploaded via HTTPS and if needed HTTP (deprecated)

A simple script like the one below can be used to upload the report

import json
import requests

URL = "tests.obspy.org"

data = json.load(open("report.json", "rt"))
response = requests.post(f"https://{URL}/post/v2/", json=data)
print("Status Code:", response.status_code)
print("Report URL:", response.content)

Please feel to already use the service and test it. I can delete reports later if needed. Network tests are not handled yet as I have no test report including those. Displaying warnings seems to me very extensive (see report above) - I could filter them for ObsPy related warnings only or drop them completely.

d-chambers commented 2 years ago

Network tests are not handled yet as I have no test report including those.

I ran all (including network) and submitted here:

https://tests.obspy.org/115766/

Also attached here:

report_all.zip

I agree on the warnings, it seems a little excessive.

trichter commented 2 years ago

Maybe we can show a warning for each combination of Category + Message once. Optionally \nand xx other locations could be added in filename column.

I would prefer the previous concise format of tracebacks.

barsch commented 2 years ago

Maybe we can show a warning for each combination of Category + Message once. Optionally \nand xx other locations could be added in filename column.

I will remove duplicates within the warnings - lets see how this is looking

I would prefer the previous concise format of tracebacks.

me actually too - I'm no fan of the current error output and especially size - however looking at the traceback array given within the submitted JSON file it seems there is some lines missing compared to the default tracebacks we had before:

(1) old style, from https://tests.obspy.org/115744/#1

Traceback (most recent call last):
File "/home/eule/dev/obspy/obspy/io/sh/tests/test_core.py", line 40, in test_read_101_traces
fail()
File "/home/eule/dev/obspy/obspy/io/sh/tests/test_core.py", line 19, in fail
fail2()
File "/home/eule/dev/obspy/obspy/io/sh/tests/test_core.py", line 15, in fail2
1/0
ZeroDivisionError: division by zero

vs.

(2) new style, from https://tests.obspy.org/115755/json/

{"traceback": [
    {"path": "io/nlloc/tests/test_util.py", "lineno": 58, "message": ""},
    {"path": "io/nlloc/util.py", "lineno": 47, "message": "in read_nlloc_scatter"},
    {"path": "io/nlloc/tests/test_util.py", "lineno": 23, "message": "in _coordinate_conversion"},
    {"path": "/home/derrick/anaconda3/lib/python3.8/site-packages/pyproj/proj.py", "lineno": 107, "message": "in __init__"},
    {"path": "/home/derrick/anaconda3/lib/python3.8/site-packages/pyproj/crs/crs.py", "lineno": 440, "message": "in from_user_input"},
    {"path": "/home/derrick/anaconda3/lib/python3.8/site-packages/pyproj/crs/crs.py", "lineno": 296, "message": "in __init__"},
    {"path": "pyproj/_crs.pyx", "lineno": 2338, "message": "CRSError"}
]}

There are options to use the default python traceback as output, see https://docs.pytest.org/en/latest/how-to/output.html - maybe we can play with that?

d-chambers commented 2 years ago

Ok, I added --tb=native when running obspy-runtests and the result looks like this:

https://tests.obspy.org/115923/#1

Should be what we are looking for no?

barsch commented 2 years ago

perfect!

barsch commented 2 years ago

(1) alright removing duplicated warnings helps - https://tests.obspy.org/115942/ has been reduced from 3268 warnings to 79 -> it would be probably smart to do this on client side in order to reduce the overall file size

(2) old style traceback formatting has been re-enabled, see https://tests.obspy.org/115942/#1

d-chambers commented 2 years ago

it would be probably smart to do this on client side in order to reduce the overall file size

Done. Ok I think we are getting close to merging the changes. Anything else you want to see before we do?

barsch commented 2 years ago

I only need some reports with network failures, so I can reimplement this feature. I also assume after reading https://github.com/obspy/obspy/issues/2930#issuecomment-1008931066 image comparison is not needed any more?

trichter commented 2 years ago

See new reports at https://tests.obspy.org/?pr=2489

I also assume after reading obspy/obspy#2930 (comment) image comparison is not needed any more?

Yes, but I think it is planned to be reintroduced with pytest-mpl.

barsch commented 2 years ago

alright network failures are reimplemented, see e.g. https://tests.obspy.org/116094/

Closing the ticket for now. Feel free to open a new ticket when image comparison is reintroduced and should be part of the test reports and ofc if you find any issues.