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

Remove unittest #3245

Merged
merged 31 commits into from Jan 18, 2023
Merged

Remove unittest #3245

merged 31 commits into from Jan 18, 2023

Conversation

megies
Copy link
Member

@megies megies commented Dec 20, 2022

What does this PR do?

Get rid of unittest from the whole test code base and replace with pytest.
(Except for mock module which comes in unittest)

Generally this would belong into master but if we do it there we end up with a massive diff between maintenance and master branch and will end up in merge hell most likely a lot of times when merging things back into master from maintenance.

Why was it initiated? Any relevant Issues?

Make testing code base more consistent.

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • While the PR is still work-in-progress, the no_ci label can be added to skip CI builds
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just add the build_docs tag to this PR.
    Docs will be served at docs.obspy.org/pr/{branch_name} (do not use master branch).
    Please post a link to the relevant piece of documentation.
  • If all tests including network modules (e.g. clients.fdsn) should be tested for the PR,
    just add the test_network tag to this PR.
  • All tests still pass.
  • Any new features or fixed regressions are covered via new tests.
  • Any new or changed features are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .
  • If the changes affect any plotting functions you have checked that the plots
    from all the CI builds look correct. Add the "upload_plots" tag so that plotting
    outputs are attached as artifacts.
  • New modules, add the module to CODEOWNERS with your github handle
  • Add the ready for review tag when you are ready for the PR to be reviewed.

@megies megies added testing issues generally related to our testing setup / infrastructure test_network tell github actions to also run network tests for this PR labels Dec 20, 2022
@megies megies added this to the 1.4.1 milestone Dec 20, 2022
@megies
Copy link
Member Author

megies commented Dec 20, 2022

Sorry about all the pings through CODEOWNERS, can't deselect people in the "new PR" thingy, apparently

@megies
Copy link
Member Author

megies commented Dec 20, 2022

This is basically..

  • ran unittest2pytest on code base
  • ran another self made quick-n-dirty script to do things that the above didn't do (see below)
  • manual fixes
import re
import sys

path = sys.argv[1]

lines = []
with open(path, "rt", encoding="UTF-8") as fh:
    for line in fh:
        # remove unittest imports
        if line.strip() == 'import unittest':
            continue
        if line.strip() == 'from unittest import mock':
            continue
        # remove inheritance from unittest on test classes
        if re.match(r'class [^(]+\(unittest\.TestCase\):', line):
            line = re.sub(r'\(unittest\.TestCase\):', '():', line, count=1)
        # remove "suite" setup
        if re.match(r'def suite\(', line):
            while not line.startswith('    return '):
                line = fh.readline()
            continue
        # remove hook for CLI execution of tests by calling file
        if re.match(r"if __name__ == (['\"])__main__\1:", line):
            line = fh.readline()
            while line.startswith("    "):
                line = fh.readline()
                continue
        # replace skip decorator
        if re.search(r'@unittest.skipIf\(', line):
            line = re.sub(r'@unittest.skipIf\(([^,]*), (.*)\)', r'@pytest.mark.skipif(\1, reason=\2)', line)
            line = re.sub(r'@unittest.skipIf\(([^,]*)\)', r'@pytest.mark.skipif(\1)', line)
        lines.append(line)

# remove empty lines at end (due to some content line removals around end of
# file (suite / __main__)
while not lines[-1].strip():
    lines.pop()

for line in lines:
    if "unittest" in lines:
        print(f'found "unittest" in path "{path}"')
        break

with open(path, "wt", encoding="UTF-8") as fh:
    fh.writelines(lines)

@megies
Copy link
Member Author

megies commented Dec 20, 2022

Oh.. wasn't aware that the linter isn't run with obspy-runtests anymore.. looks like some cleanup to do 🤣

@trichter
Copy link
Member

trichter commented Dec 20, 2022

Nice! I think you do not want to remove the mock line with

        if line.strip() == 'from unittest import mock':
            continue

@megies
Copy link
Member Author

megies commented Dec 21, 2022

Nice! I think you do not want to remove the mock line with

Yea I realized that one.. 🤣 just added those few ones back manually

Copy link
Member

@trichter trichter left a comment

Choose a reason for hiding this comment

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

I skimmed through the changes. Looks OK to me!

@megies
Copy link
Member Author

megies commented Dec 21, 2022

Not yet ready.. more transistioning to do 😬

@megies megies marked this pull request as draft December 21, 2022 16:24
@megies
Copy link
Member Author

megies commented Dec 21, 2022

Holy crap, wouldn't have touched this had I known how much of a rabbit hole this was

@trichter
Copy link
Member

-    def setUp(self):
+    @classmethod
+    def setup_class(cls):

setUp was run before each test method, setup_class is probably run only once before all test methods -- which makes more sense IMHO, but might break some tests.

@megies
Copy link
Member Author

megies commented Dec 22, 2022

Ah right, I thought they were run once, that mustve been some other unittest construct I somehow remembered

@trichter
Copy link
Member

Commit f2162f8 can be removed again to get rid of the merge conflict, it is covered by #3247.

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Gave it a reasonable skim and noted some things that could be improved. Probably missed several cases that are similar, and these could conceivably be in a followup PR instead.

obspy/clients/fdsn/tests/test_base_routing_client.py Outdated Show resolved Hide resolved
obspy/clients/fdsn/tests/test_base_routing_client.py Outdated Show resolved Hide resolved
obspy/clients/fdsn/tests/test_base_routing_client.py Outdated Show resolved Hide resolved
obspy/clients/fdsn/tests/test_base_routing_client.py Outdated Show resolved Hide resolved
obspy/clients/fdsn/tests/test_base_routing_client.py Outdated Show resolved Hide resolved
obspy/clients/fdsn/tests/test_mass_downloader.py Outdated Show resolved Hide resolved
obspy/clients/nrl/tests/test_nrl.py Outdated Show resolved Hide resolved
obspy/clients/syngine/tests/test_client.py Outdated Show resolved Hide resolved
obspy/io/ah/tests/test_xdrlib.py Show resolved Hide resolved
obspy/io/arclink/tests/test_inventory_xml.py Outdated Show resolved Hide resolved
@megies
Copy link
Member Author

megies commented Jan 12, 2023

@trichter is the test_network label off? It seems some runners only run network tests and not the non-network ones: https://tests.obspy.org/?pr=3245

@megies megies marked this pull request as ready for review January 12, 2023 14:52
@megies megies added the ready for review PRs that are ready to be reviewed to get marked ready to merge label Jan 12, 2023
@megies
Copy link
Member Author

megies commented Jan 12, 2023

@QuLogic found many more occurences of message checking in newly refactored pytest.raises 👍

@trichter
Copy link
Member

trichter commented Jan 12, 2023

@trichter is the test_network label off? It seems some runners only run network tests and not the non-network ones: https://tests.obspy.org/?pr=3245

That is the expected behavior. The test_network label adds runs with the --network flag not the --all flag. The non-network tests are already covered by the other runs. Why add more overhead? By the way, would be nice to have a way to see some of the test CLI flags (warnings, all, network) on Obspy's test report site and maybe even on the test overview page.

@megies
Copy link
Member Author

megies commented Jan 12, 2023

The test_network label adds runs

Ah gotcha, I thought it just ran all tests on existing runners

- unify path lookups of test files with fixtures
- use pathlib more for path building
- some other minor changes, get rid of unittest style test case setups etc.
- use match in pytest.raises everywhere
- set literals
@megies
Copy link
Member Author

megies commented Jan 12, 2023

By the way, would be nice to have a way to see some of the test CLI flags (warnings, all, network) on Obspy's test report site and maybe even on the test overview page.

They can be extracted from the json so should not be to hard to do. You mean like filter for network test runs? Then it would mean changing the database structure probably.

Screenshot from 2023-01-12 18-05-25

@trichter
Copy link
Member

They can be extracted from the json so should not be to hard to do. You mean like filter for network test runs? Then it would mean changing the database structure probably.

Yep, I looked them up in the JSON from time to time.

A filter would be great. And/or a new column, this could simply have a flags header with values '', 'n', 'a', 'w', 'nw', 'aw'. But nor sure how to do the filtering.

@trichter
Copy link
Member

trichter commented Jan 17, 2023

The PR looks good to me!

@megies megies merged commit 332370c into maintenance_1.4.x Jan 18, 2023
@megies megies deleted the remove_unittest branch January 18, 2023 08:54
@megies
Copy link
Member Author

megies commented Jan 18, 2023

Did just merge maintenance branch into master and pushed it, so both will be easily merge-able going forward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review PRs that are ready to be reviewed to get marked ready to merge test_network tell github actions to also run network tests for this PR testing issues generally related to our testing setup / infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants