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

Bugfix MTE-2547 Use the latest version of PyFxA and pytest-fxa #22179

Merged
merged 5 commits into from
Sep 26, 2024

Conversation

clarmso
Copy link
Collaborator

@clarmso clarmso commented Sep 24, 2024

📜 Tickets

Jira ticket

💡 Description

The Sync Integration Tests have been failing since March 2024 due to an API change in account deletion: Since then the destroy_account API requires a token. Such a change caused the Mozilla accounts that were created during test execution cannot be removed at test tearDown(). The logs of the test run shows the following error:

E           fxa.errors.ClientError: Invalid authentication token: Missing authentication

The latest main (not released yet!) of PyFxA enables us to remove Mozilla accounts using the latest API change. I published the package in PyPI (note "-mte" suffix): https://pypi.org/project/pytest-fxa-mte/

Then I import the PyFxA-mte package to pytest-fxa-mte. pytest-fxa-mte is published in PyPI: https://test.pypi.org/project/pytest-fxa-mte/.

Finally, I pinned the version of pytest-fxa-mte as a part of the dependencies of Sync Integration Tests. 🎉

I have tested my changes using the following command:

pipenv install
pipenv run pytest \
    test_integration.py::test_sync_tabs_from_device \
    test_integration.py::test_sync_tabs_from_desktop \
    test_integration.py::test_sync_bookmark_from_device \
    test_integration.py::test_sync_bookmark_from_desktop \
    test_integration.py::test_sync_history_from_device \
    test_integration.py::test_sync_tabs_from_device \
    test_integration.py::test_sync_history_from_desktop \
    test_integration.py::test_sync_logins_from_desktop \
    test_integration.py::test_sync_tabs_from_desktop \
    test_integration.py::test_sync_disconnect_connect_fxa \
    test_integration.py::test_sync_china_fxa_server \
    --tps ../../../../test-fixtures/tps.xpi \
    --color=yes --junit-xml=results/junit.xml --html=results/index.html

I confirmed that the accounts that were created during the test run no longer exist on stage. The following INFO log is seeen:

INFO     root:plugin.py:60 Removed: FxAccount(email='[email protected]', password='oBApqnXp')

In addition, the following snippet of code is run under the virtualenv of PyFxA to confirm that the account no longer exists.

from fxa.core import Client
client = Client("https://api-accounts.stage.mozaws.net")
client.destroy_account(<email>,<password>)

🤔 Unintentional change: Other dependencies including pytest-html has been bumped. 🤔

📝 Checklist

You have to check all boxes before merging

  • Filled in the above information (tickets numbers and description of your work)
  • Updated the PR name to follow our PR naming guidelines
  • Wrote unit tests and/or ensured the tests suite is passing
  • When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • If needed, I updated documentation / comments for complex code and public methods
  • If needed, added a backport comment (example @Mergifyio backport release/v120)

@clarmso clarmso requested a review from a team as a code owner September 24, 2024 21:02
@@ -1,7 +1,7 @@
{
"_meta": {
"hash": {
"sha256": "5c8b5b678f28852600d42fccc45bc6387f68bccea6613819686d72b66ef7b1f2"
"sha256": "710b2b26f95df49fcc12cc73f8922fe70dca0ebb74636b2099b189499bdb9391"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we don't like these changes, I can roll back everything except the ones related to pytest-fxa-mte and pyfxa-mte. What do you all think?

Since Pipfile does not pin 📌 particular versions of depdencies, pipenv install bumps the versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

That probably makes sense, you only want changes that reflect changes in this commit.

@@ -11,7 +11,7 @@ mozprofile = "*"
mozrunner = "*"
mozversion = "*"
pytest = "*"
pytest-fxa = "*"
pytest-fxa-mte = "1.5.1"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@clarmso clarmso added the Do Not Merge ⛔️ This issue is a work in progress and is not ready to land label Sep 24, 2024
@clarmso
Copy link
Collaborator Author

clarmso commented Sep 24, 2024

I added "do not merge" because I am having Python environment issues on MacStadium.

@clarmso clarmso removed the Do Not Merge ⛔️ This issue is a work in progress and is not ready to land label Sep 24, 2024
@clarmso
Copy link
Collaborator Author

clarmso commented Sep 24, 2024

I added "do not merge" because I am having Python environment issues on MacStadium.

I have found out that --python 3.12 option must be specified. My versions of the packages were worked on using Python 3.12.

I have tested the command on MacStadium as well.

@AaronMT
Copy link
Contributor

AaronMT commented Sep 25, 2024

That's great Clare, this looks like the path forward.

For https://pypi.org/project/pytest-fxa-mte/ can you modify the product description to indicate that it's a fork with updates to handling authentication?

Is it safe to drop support for all those older versions of Python in your fork that are mentioned, as it shows in the description on your fork Is that configurable in the PyPi project since you're the maintainer?

@clarmso clarmso merged commit dc21271 into main Sep 26, 2024
5 checks passed
@clarmso clarmso deleted the cs/MTE-2547-bump-pyfxa-mte branch September 26, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants