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

DM-7207: Pytest updates #84

Merged
merged 135 commits into from Aug 30, 2016
Merged

DM-7207: Pytest updates #84

merged 135 commits into from Aug 30, 2016

Conversation

timj
Copy link
Member

@timj timj commented Aug 17, 2016

No description provided.

@timj timj mentioned this pull request Aug 23, 2016
@parejkoj
Copy link
Contributor

I just want to register my agreement that random seeding should go in setUp: you don't want any dependence on other tests being run, so you want to reset the seed every time.

if __name__ == "__main__":
run(True)
lsst.utils.tests.init()
unittest.main()
Copy link
Member Author

Choose a reason for hiding this comment

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

Needs newline

@timj
Copy link
Member Author

timj commented Aug 25, 2016

There are a few bare assertRaises in the code (some of which have lambda). I imagine @jdswinbank doesn't want us to spend more time on fixing them so can you please file a ticket so we can remember to clean them up.

@@ -308,11 +275,6 @@ def defineFilterProperty(self, name, lambdaEff, force=False):
def testListFilters(self):
self.assertEqual(afwImage.Filter_getNames(), self.filters)

def testCtor(self):
Copy link
Member Author

Choose a reason for hiding this comment

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

Why has this test been deleted?

Copy link
Contributor

Choose a reason for hiding this comment

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

There are no asserts and there are many other tests that make exactly that function call. It's not clear what the failure condition for this test would be, anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

As I say above, asserts are not required in test code. It may be that the test code used to throw an exception but then something was fixed. Of course, if everything that is in this test is found in other tests then it is no longer needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this functionality is already in several other tests, including the an exact replica in testFilterEquality, so I think it's ok to leave this deleted.

@fred3m
Copy link
Contributor

fred3m commented Aug 30, 2016

Yes @timj, I'm not sure what happened there.

@fred3m fred3m merged commit ee8608d into master Aug 30, 2016
@ktlim ktlim deleted the tickets/DM-7207 branch August 25, 2018 06:44
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

5 participants