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

Some initial unit tests for mpathcount.py #500

Merged
merged 3 commits into from
Jun 29, 2020

Conversation

TimSmithCtx
Copy link
Contributor

No description provided.

Signed-off-by: Tim Smith <tim.smith@citrix.com>
@coveralls
Copy link

coveralls commented May 19, 2020

Coverage Status

Coverage increased (+0.9%) to 40.296% when pulling fb526db on TimSmithCtx:private/timsmi/CP-33617 into 45c4009 on xapi-project:master.

Copy link
Contributor

@MarkSymsCtx MarkSymsCtx left a comment

Choose a reason for hiding this comment

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

generally looks good. A few minor niggles about using unittest asserts.

@@ -24,6 +24,19 @@
import glob
import json

supported = ['iscsi','lvmoiscsi','rawhba','lvmohba', 'ocfsohba', 'ocfsoiscsi', 'netapp','lvmofcoe', 'gfs2']
Copy link
Contributor

Choose a reason for hiding this comment

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

missing space after netapp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was in the original. Will fix in the commit which moves it.

" `- 7:0:1:4 sdam 66:96 failed faulty running"
]
count, total = mpathcount.get_path_count('3600a098038303973743f486833396d44')
assert(total == 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

self.assertEqual(4, total, msg='XXX')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did these things. New version pushed

mpathcount.MAPPER_DIR = "test_support/fake_mapper"

mpathcount.update_config("fred", "3600a098038303973743f486833396d44", "[2, 4]", remove, add, True)
assert(store['MPPEnabled'])
Copy link
Contributor

Choose a reason for hiding this comment

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

self.assertIsNotNone(store['MPPEnabled'])

https://docs.python.org/3/library/unittest.html#assert-methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did these things. New version pushed

Move some of the functionality of mpathcount.py into functions
to make it a little easier to write unit tests for it

Signed-off-by: Tim Smith <tim.smith@citrix.com>
import SR
import util

#class FakeXapi(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove commented out code.

" `- 7:0:1:4 sdam 66:96 failed faulty running"
]
count, total = mpathcount.get_path_count('3600a098038303973743f486833396d44')
self.assertEqual(4, total, msg='total should be 4, was {}'.format(total))
Copy link
Contributor

Choose a reason for hiding this comment

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

the format is unnecessary, the unit test assert does all that anyway. Something like "Total count incorrect" is sufficient

Signed-off-by: Tim Smith <tim.smith@citrix.com>
Copy link
Contributor

@MarkSymsCtx MarkSymsCtx left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@BenSimsCitrix BenSimsCitrix left a comment

Choose a reason for hiding this comment

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

LGTM

@MarkSymsCtx MarkSymsCtx merged commit bd2d872 into xapi-project:master Jun 29, 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.

None yet

4 participants