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

Use pytest #48

Closed
wants to merge 24 commits into from
Closed

Use pytest #48

wants to merge 24 commits into from

Conversation

quantum9Innovation
Copy link
Member

@quantum9Innovation quantum9Innovation commented May 9, 2021

Set up pytest as main testing framework

After PR #46, solves issue #47


New Feature

Transforms old tests to use pytest as the main testing framework.
pytest allows for

  • more testing accuracy
  • easily accessible testing suite
  • fast testing due to local cache
  • assert-based testing and bounds
    It also adds GitHub CODEOWNERS

Known Issues

Fixes #47 "Getting started with pytest"

Code Breakdown

  • .github/CODEOWNERS
    • Adds CODEOWNERS
  • .github/workflows/
    • build.yml, coverage.yml
      • Changed testing to use $ python -m pytest command
    • Optimizes workflows (@Quantalabs check optimizations)
  • bin/epispot
    • Replace broad except with except ImportError
  • bin/requirements.txt
    • Removes epispot due to pip install -r bin/requirements.txt command in workflows which installs epispot from PyPI instead of from the source
  • epispot/init.py
    • Add shortened README and guide to top of file (for help() commands)
    • Add dependency and sanity checks (@Quantalabs please see)
    • Remove __ from metadata variable names to make them globally accessible
    • Add documentation on metadata
  • epispot/
    • Add # pragma: no cover on soon-to-be deprecated functions
  • explorables/
    • Remove all tests, former testing suites, examples, and tutorials and move them to this directory to keep tests/ directory clean
  • tests/
    • Replace testing suite with new one that uses pytest
  • release.py
    • Update to be compatible with new __init__.py
  • setup.py, setup-nightly.py
    • Add more metadata and classifier information

Additional Notes

There's a lot going on in this PR and it may need to be broken up into multiple PRs.
For now, it will be created as a draft PR just because of the sheer scale of the PR.
Additionally, before moving into merge stage, all merge conflicts should be resolved and all basic checks (build and code coverage) should pass. After we enter the merge stage, reviewers should analyze code for compatibility issues and other code quality-related alerts will be dealt with if necessary.


Closes #47

- release.py can now update new `epispot/__init__.py`
- Adds classifiers and project URLS to both `setup.py` and `setup-nightly.py`
- Removes unnecessary testing suites (`test_cov.py`)
- Changes `epispot.__version__` to `epispot.version`, meaning it is now accessible globally and will show up on pdoc
    - Updates `bin/epispot` to use new `epispot.version` to give version info
Replaces with:
```python
except ImportError:
    # foo
```
Creates README.md to explain testing with `pytest`

See `test_x.py` as an example of a properly-configured test.
All `epispot.pre` modules are now compatible with pytest through `tests/pytest/test_sir-variants.py`
- SIR
- SEIR
- SIRD
- SIHRD
Tests
- Recurrent models
- Starting state
- Critical compartment
- Triage
Tests
- Plotting
- Fitters (none currently)
Removes
 - epi.fitters
     - grad_des
     - tree_search (experimental)
 - epi.plots
     - compare
Changes most foo/* paths to foo/
- Fixes code coverage issue due to warnings being included in code coverage reporting
- Changes sanity check to check *only* for missing version and source information
- Changes `setup-nightly` package data
* Removes existing epispot installation in testing workflows
Solves installation issue

* Bypasses Python `proceed? (y/n)` prompt when uninstalling epispot
Fixes error from previous commit

* Runs `pytest` as module to import epispot from `Namespace`

* Removes epispot from `bin/requirements.txt`
Per @Quantalabs request
@quantum9Innovation quantum9Innovation added doc 📜 Improvements or additions to documentation feat 🚀 New feature or request high-priority 🔼 This is important labels May 9, 2021
@quantum9Innovation quantum9Innovation self-assigned this May 9, 2021
@quantum9Innovation quantum9Innovation added this to Needs triage in Management v2 via automation May 9, 2021
@github-actions github-actions bot added src Changes to the package source code tests Testing suite updates repo Changes to workflows/repo files labels May 9, 2021
@codecov
Copy link

codecov bot commented May 9, 2021

Codecov Report

Merging #48 (0fc625a) into master (009d5f2) will decrease coverage by 9.02%.
The diff coverage is 25.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
- Coverage   97.44%   88.42%   -9.03%     
==========================================
  Files           6        7       +1     
  Lines         470      432      -38     
==========================================
- Hits          458      382      -76     
- Misses         12       50      +38     
Impacted Files Coverage Δ
bin/bcolors.py 0.00% <0.00%> (ø)
epispot/fitters.py 100.00% <ø> (ø)
epispot/__init__.py 100.00% <100.00%> (ø)
epispot/plots.py 100.00% <100.00%> (ø)
epispot/comps.py 96.03% <0.00%> (+0.79%) ⬆️

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 99e1f24...0fc625a. Read the comment docs.

@quantum9Innovation
Copy link
Member Author

Code coverage failed due to not testing the CLI, which will be fixed in a later PR. For now, code coverage is essentially passing.
Opening for review after:

  • LGTM Analysis (exit code irrelevant ✅ )
  • CodeQL Analysis (must pass ✔️ )

@quantum9Innovation quantum9Innovation moved this from Needs triage to High priority in Management v2 May 9, 2021
@lgtm-com
Copy link

lgtm-com bot commented May 9, 2021

This pull request introduces 1 alert and fixes 1 when merging 0fc625a into 99e1f24 - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

fixed alerts:

  • 1 for Except block handles 'BaseException'

@quantum9Innovation quantum9Innovation marked this pull request as ready for review May 9, 2021 22:12
@quantum9Innovation
Copy link
Member Author

We are now ready for review, pinging @Quantalabs

.github/workflows/coverage.yml Show resolved Hide resolved
.github/workflows/vulnerability.yml Show resolved Hide resolved
tests/test_advanced.py Show resolved Hide resolved
tests/test_sir-variants.py Show resolved Hide resolved
Management v2 automation moved this from High priority to Low priority May 10, 2021
@quantum9Innovation
Copy link
Member Author

This is still high priority

@quantum9Innovation
Copy link
Member Author

After review, we're splitting up the PR into multiple PRs that will be merged in a certain order specified on this PR and the Management project

@quantum9Innovation quantum9Innovation added the duplicate This issue or pull request already exists label May 13, 2021
@quantum9Innovation quantum9Innovation moved this from Low priority to High priority in Management v2 May 13, 2021
Management v2 automation moved this from High priority to Closed May 20, 2021
@quantum9Innovation quantum9Innovation deleted the testing branch May 22, 2021 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc 📜 Improvements or additions to documentation duplicate This issue or pull request already exists feat 🚀 New feature or request high-priority 🔼 This is important repo Changes to workflows/repo files src Changes to the package source code tests Testing suite updates
Projects
No open projects
Management v2
  
Closed
Development

Successfully merging this pull request may close these issues.

Get testing with pytest
2 participants