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

Fix failing app tests #19971

Merged
merged 7 commits into from
Jun 13, 2024
Merged

Fix failing app tests #19971

merged 7 commits into from
Jun 13, 2024

Conversation

awaelchli
Copy link
Contributor

@awaelchli awaelchli commented Jun 12, 2024

What does this PR do?

App tests have failures that block the release.

  1. XFAIL was strict, so when the test starts working it would be marked as failed.
  2. The package urllib3 >= 2.0 gets installed in some CI jobs. We have a test that mocks the internals but it's not valid anymore for this version:

@mock.patch("urllib3.connectionpool.HTTPConnectionPool._get_conn")
def test_http_client_retry_post(getconn_mock):
getconn_mock.return_value.getresponse.side_effect = [
mock.Mock(status=500, msg=HTTPMessage()),
mock.Mock(status=599, msg=HTTPMessage()),
mock.Mock(status=405, msg=HTTPMessage()),
mock.Mock(status=200, msg=HTTPMessage()),
]
client = HTTPClient(base_url="http://test.url")
r = client.post("/test")
r.raise_for_status()
assert getconn_mock.return_value.request.mock_calls == [
mock.call("POST", "/test", body=None, headers=mock.ANY),
mock.call("POST", "/test", body=None, headers=mock.ANY),
mock.call("POST", "/test", body=None, headers=mock.ANY),
mock.call("POST", "/test", body=None, headers=mock.ANY),
]

I tried to adjust the mocks for a while but couldn't figure out how to do it correctly for the new version. For now I can suggest restricting the urllib3 version to < 2.0.


📚 Documentation preview 📚: https://pytorch-lightning--19971.org.readthedocs.build/en/19971/

cc @Borda

@github-actions github-actions bot added app (removed) Generic label for Lightning App package dependencies Pull requests that update a dependency file labels Jun 12, 2024
@awaelchli awaelchli added bug Something isn't working tests and removed app (removed) Generic label for Lightning App package dependencies Pull requests that update a dependency file labels Jun 12, 2024
@awaelchli awaelchli added this to the 2.3 milestone Jun 12, 2024
Copy link
Contributor

github-actions bot commented Jun 12, 2024

⚡ Required checks status: All passing 🟢

Groups summary

🟢 lightning_app: Tests workflow
Check ID Status
app-pytest (macOS-11, lightning, 3.8, latest) success
app-pytest (macOS-11, lightning, 3.8, oldest) success
app-pytest (macOS-11, app, 3.9, latest) success
app-pytest (macOS-12, app, 3.11, latest) success
app-pytest (ubuntu-20.04, lightning, 3.8, latest) success
app-pytest (ubuntu-20.04, lightning, 3.8, oldest) success
app-pytest (ubuntu-20.04, app, 3.9, latest) success
app-pytest (ubuntu-22.04, app, 3.11, latest) success
app-pytest (windows-2022, lightning, 3.8, latest) success
app-pytest (windows-2022, lightning, 3.8, oldest) success
app-pytest (windows-2022, app, 3.8, latest) success
app-pytest (windows-2022, app, 3.11, latest) success

These checks are required after the changes to tests/tests_app/cli/test_cmd_launch.py, tests/tests_app/core/test_lightning_api.py, tests/tests_app/core/test_lightning_app.py, tests/tests_app/utilities/test_network.py, requirements/app/app.txt.

🟢 lightning_app: Examples
Check ID Status
app-examples (macOS-11, lightning, 3.9, latest) success
app-examples (macOS-11, lightning, 3.9, oldest) success
app-examples (macOS-11, app, 3.9, latest) success
app-examples (ubuntu-20.04, lightning, 3.9, latest) success
app-examples (ubuntu-20.04, lightning, 3.9, oldest) success
app-examples (ubuntu-20.04, app, 3.9, latest) success
app-examples (windows-2022, lightning, 3.9, latest) success
app-examples (windows-2022, lightning, 3.9, oldest) success
app-examples (windows-2022, app, 3.9, latest) success

These checks are required after the changes to requirements/app/app.txt.

🟢 lightning_app: Docs
Check ID Status
docs-make (app, doctest) success
docs-make (app, html) success

These checks are required after the changes to requirements/app/app.txt.

🟢 mypy
Check ID Status
mypy success

These checks are required after the changes to requirements/app/app.txt.

🟢 install
Check ID Status
install-pkg (ubuntu-22.04, app, 3.8) success
install-pkg (ubuntu-22.04, app, 3.11) success
install-pkg (ubuntu-22.04, fabric, 3.8) success
install-pkg (ubuntu-22.04, fabric, 3.11) success
install-pkg (ubuntu-22.04, pytorch, 3.8) success
install-pkg (ubuntu-22.04, pytorch, 3.11) success
install-pkg (ubuntu-22.04, lightning, 3.8) success
install-pkg (ubuntu-22.04, lightning, 3.11) success
install-pkg (ubuntu-22.04, notset, 3.8) success
install-pkg (ubuntu-22.04, notset, 3.11) success
install-pkg (macOS-12, app, 3.8) success
install-pkg (macOS-12, app, 3.11) success
install-pkg (macOS-12, fabric, 3.8) success
install-pkg (macOS-12, fabric, 3.11) success
install-pkg (macOS-12, pytorch, 3.8) success
install-pkg (macOS-12, pytorch, 3.11) success
install-pkg (macOS-12, lightning, 3.8) success
install-pkg (macOS-12, lightning, 3.11) success
install-pkg (macOS-12, notset, 3.8) success
install-pkg (macOS-12, notset, 3.11) success
install-pkg (windows-2022, app, 3.8) success
install-pkg (windows-2022, app, 3.11) success
install-pkg (windows-2022, fabric, 3.8) success
install-pkg (windows-2022, fabric, 3.11) success
install-pkg (windows-2022, pytorch, 3.8) success
install-pkg (windows-2022, pytorch, 3.11) success
install-pkg (windows-2022, lightning, 3.8) success
install-pkg (windows-2022, lightning, 3.11) success
install-pkg (windows-2022, notset, 3.8) success
install-pkg (windows-2022, notset, 3.11) success

These checks are required after the changes to requirements/app/app.txt.


Thank you for your contribution! 💜

Note
This comment is automatically generated and updates for 60 minutes every 180 seconds. If you have any other questions, contact carmocca for help.

@github-actions github-actions bot added app (removed) Generic label for Lightning App package dependencies Pull requests that update a dependency file labels Jun 12, 2024
@mergify mergify bot added the ready PRs ready to be merged label Jun 12, 2024
@tchaton tchaton merged commit a42484c into master Jun 13, 2024
82 checks passed
@tchaton tchaton deleted the tests/fix-app-tests branch June 13, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app (removed) Generic label for Lightning App package bug Something isn't working dependencies Pull requests that update a dependency file ready PRs ready to be merged tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants