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

Tangodict code improvements #187

Merged
merged 8 commits into from
Dec 13, 2020
Merged

Tangodict code improvements #187

merged 8 commits into from
Dec 13, 2020

Conversation

clairecw
Copy link
Contributor

@clairecw clairecw commented Dec 10, 2020

Implements contains in the Tango*Dictionary classes, replacing any unnecessary calls to .keys().
Also adds tests for TangoDictionary accordingly.

Fixes #171.

@fanpu
Copy link
Contributor

fanpu commented Dec 11, 2020

Hi @clairecw , thank you for your PR! It looks really good in general, and I really appreciate the fact that there are unit tests. A couple of more complicated unit tests would also be helpful. Do you have other instructions for end-to-end testing that you could share as well?

@clairecw
Copy link
Contributor Author

Hi @fanpu, thanks so much for the feedback! Yes, I've added unit tests testing a bit more functionality for the dictionary object specifically - if you could TAL when you get the chance and see if there's anything else you had in mind that'd be awesome!
Lmk also if you'd like to see unit tests for the other Tango objects.

As for E2E tests, I suppose there could be tests for each of the functions in TangoServer, and maybe another file testing the endpoints themselves.

@fanpu
Copy link
Contributor

fanpu commented Dec 12, 2020

Great job and thanks! I've looked through the changes and tests and they look good, and I've also tested it with Autolab.

One last change before it's ready to be merged: could you use the unittest methods for asserting the tests instead of the native Python assert? You can refer to this file for examples.

@fanpu
Copy link
Contributor

fanpu commented Dec 12, 2020

Tests for other Tango objects would also definitely be greatly appreciated, see #183 for some ideas

Just to clarify, those tests would be better for a separate pr if you are intending to work on them, the pr right now is good apart from the assert changes

tests/testObjects.py Outdated Show resolved Hide resolved
@clairecw clairecw requested a review from fanpu December 13, 2020 02:45
Copy link
Contributor

@fanpu fanpu left a comment

Choose a reason for hiding this comment

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

Excellent job and thanks once again for your contributions!

@fanpu fanpu merged commit cd256c7 into autolab:master Dec 13, 2020
@clairecw clairecw deleted the tangodict branch December 15, 2020 16:40
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.

Code improvement opportunities
2 participants