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

Refactor sanity checks to use pytest #2221

Merged
merged 9 commits into from
Mar 11, 2024
Merged

Conversation

mreso
Copy link
Collaborator

@mreso mreso commented Apr 5, 2023

Description

This PR refactors parts of our sanity checks to use pytest infra which enables devs to test model registration + snapshotting using

pytest test/pytest/sanity

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Feature/Issue validation/testing

Please describe the Unit or Integration tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

  • pytest test/pytest/sanity
========================================================================================================================================================================== test session starts ===========================================================================================================================================================================
platform linux -- Python 3.9.16, pytest-7.2.2, pluggy-1.0.0
rootdir: /home/ubuntu/serve
plugins: mock-3.10.0, cov-4.0.0
collected 22 items

test/pytest/sanity/test_config_snapshot.py .                                                                                                                                                                                                                                                                                                                       [  4%]
test/pytest/sanity/test_model_registering.py .....................                                                                                                                                                                                                                                                                                                 [100%]/home/ubuntu/miniconda3/envs/microbatching/lib/python3.9/site-packages/coverage/control.py:858: CoverageWarning: No data was collected. (no-data-collected)
  self._warn("No data was collected.", slug="no-data-collected")


---------- coverage: platform linux, python 3.9.16-final-0 -----------
Coverage XML written to file test/pytest/sanity/coverage.xml


===================================================================================================================================================================== 22 passed in 307.21s (0:05:07) =====================================================================================================================================================================
  • python torchserve_sanity.py
    See PR checks

Checklist:

  • Did you have fun?
  • Have you added tests that prove your fix is effective or that this feature works?
  • Has code been commented, particularly in hard-to-understand areas?

@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #2221 (cf179e6) into master (9c52043) will decrease coverage by 0.08%.
The diff coverage is n/a.

❗ Current head cf179e6 differs from pull request most recent head f72fc13. Consider uploading reports for the commit f72fc13 to get more accurate results

@@            Coverage Diff             @@
##           master    #2221      +/-   ##
==========================================
- Coverage   72.44%   72.36%   -0.08%     
==========================================
  Files          85       85              
  Lines        3963     3963              
  Branches       58       58              
==========================================
- Hits         2871     2868       -3     
- Misses       1088     1091       +3     
  Partials        4        4              

see 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mreso mreso force-pushed the integrate_sanity_tests_with_pytest branch 2 times, most recently from ead57e6 to 5d4fc08 Compare August 24, 2023 22:36
Make sanity test run with pytest 90%

Added missing test for snapshotting

Use pytest tests in torchserve sanity checks
@chauhang chauhang added this to the v0.10.0 milestone Feb 27, 2024
@mreso mreso requested review from lxning and agunapal March 1, 2024 21:27
Copy link
Collaborator

@lxning lxning left a comment

Choose a reason for hiding this comment

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

Currently regression test run test/pytest.

With these changes, tt seems that regression test will also run test_sanity. Do we need this duplicate work?

@@ -163,51 +162,18 @@ def run_rest_test(model, register_model=True, unregister_model=True):


def test_sanity():
generate_grpc_client_stubs()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess sanity test will fail without gRPC client.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thats call is now a fixture

generate_grpc_client_stubs()

@mreso
Copy link
Collaborator Author

mreso commented Mar 9, 2024

Currently regression test run test/pytest.

With these changes, tt seems that regression test will also run test_sanity. Do we need this duplicate work?

@lxning good point! Have changes the PR so that sanity tests are ignored when regression tests run

@mreso mreso requested a review from lxning March 9, 2024 04:36
@mreso mreso enabled auto-merge March 11, 2024 17:19
@mreso mreso added this pull request to the merge queue Mar 11, 2024
Merged via the queue into master with commit c56715c Mar 11, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants