Skip to content

PYTHON-1787: fix NotMasterError wrong attribute error #450

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

Merged
merged 10 commits into from
Jul 2, 2020

Conversation

juliusgeo
Copy link
Contributor

@juliusgeo juliusgeo commented Jun 30, 2020

This is fixing the issue from the previous pull request which was caused by NotMasterError failing

https://jira.mongodb.org/browse/PYTHON-1787

@@ -114,7 +114,7 @@ class NotMasterError(AutoReconnect):
Subclass of :exc:`~pymongo.errors.AutoReconnect`.
"""
def __str__(self):
Copy link
Member

@ShaneHarvey ShaneHarvey Jun 30, 2020

Choose a reason for hiding this comment

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

As discussed earlier today, can you add a test that asserts that the "full error" string is included in str(exc) and traceback.format_exc()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@juliusgeo juliusgeo requested a review from ShaneHarvey June 30, 2020 20:27
@@ -439,6 +439,9 @@ def test_not_master_error(self):
client.pymongo_test.test.find_one_and_delete({})
except NotMasterError as exc:
error = exc.errors
import traceback
self.assertIn("full error", str(exc))
self.assertIn("full error", traceback.format_exc())
Copy link
Member

Choose a reason for hiding this comment

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

This is exactly what I had in mind. What do you think about moving these assertions to their own test cases (maybe to a new file test_errors.py)? You can use the error classes directly, like:

exc = NotMasterError(...)
self.assertIn("full error", str(exc))
try:
    raise exc
except NotMasterError:
    self.assertIn("full error", traceback.format_exc())

@juliusgeo juliusgeo requested a review from ShaneHarvey July 1, 2020 13:38
try:
raise exc
except NotMasterError:
import traceback
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this import to the top of the file?


class TestErrors(PyMongoTestCase):
def test_not_master_error(self):
exc = NotMasterError("not master test", "details")
Copy link
Member

Choose a reason for hiding this comment

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

Suggest using something like {ok: 0, errmsg:"error"} instead of "details".

unittest)

class TestErrors(PyMongoTestCase):
def test_not_master_error(self):
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add a similar test for OperationFailure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -0,0 +1,18 @@
from pymongo.errors import (AutoReconnect,
Copy link
Member

Choose a reason for hiding this comment

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

Add the copyright notice # Copyright 2020-present MongoDB, Inc. ....

@@ -0,0 +1,18 @@
from pymongo.errors import (AutoReconnect,
Copy link
Member

Choose a reason for hiding this comment

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

After importing standard python libraries but before importing pymongo can you add sys.path[0:0] = [""]. For example:

sys.path[0:0] = [""]

raise exc
except NotMasterError:
import traceback
self.assertIn("full error", traceback.format_exc())
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the if __name__ == "__main__": logic at the bottom of this file?

if __name__ == "__main__":
unittest.main()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

client_knobs,
PyMongoTestCase,
sanitize_cmd,
unittest)
Copy link
Member

Choose a reason for hiding this comment

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

Suggest removing any unused imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@juliusgeo juliusgeo changed the title fix NotMasterError issue fix NotMasterError wrong attribute error Jul 2, 2020
@juliusgeo juliusgeo changed the title fix NotMasterError wrong attribute error PYTHON-1787: fix NotMasterError wrong attribute error Jul 2, 2020
Copy link
Contributor Author

@juliusgeo juliusgeo left a comment

Choose a reason for hiding this comment

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

This is ready for review again

@@ -114,7 +114,7 @@ class NotMasterError(AutoReconnect):
Subclass of :exc:`~pymongo.errors.AutoReconnect`.
"""
def __str__(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

client_knobs,
PyMongoTestCase,
sanitize_cmd,
unittest)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

unittest)

class TestErrors(PyMongoTestCase):
def test_not_master_error(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

raise exc
except NotMasterError:
import traceback
self.assertIn("full error", traceback.format_exc())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

LGTM!

@juliusgeo juliusgeo merged commit a075eb7 into mongodb:master Jul 2, 2020
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