Skip to content

Change assert_ checks to assertEquals/NotEquals/True/False...#2086

Merged
tardyp merged 5 commits intobuildbot:masterfrom
ewongbb:ticket2521
Apr 3, 2016
Merged

Change assert_ checks to assertEquals/NotEquals/True/False...#2086
tardyp merged 5 commits intobuildbot:masterfrom
ewongbb:ticket2521

Conversation

@ewongbb
Copy link
Copy Markdown
Contributor

@ewongbb ewongbb commented Apr 1, 2016

This is for ticket 2521.

@codecov-io
Copy link
Copy Markdown

Current coverage is 84.26%

Merging #2086 into master will not affect coverage as of 78108f9

@@            master   #2086   diff @@
======================================
  Files          332     332       
  Stmts        31898   31898       
  Branches         0       0       
  Methods          0       0       
======================================
  Hit          26880   26880       
  Partial          0       0       
  Missed        5018    5018       

Review entire Coverage Diff as of 78108f9


Uncovered Suggestions

  1. +0.11% via ...ot/status/builder.py#384...417
  2. +0.09% via ...ot/status/builder.py#456...484
  3. +0.09% via ...dbot/changes/mail.py#480...507
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.


# The first time, it just learns the change to start at.
self.assert_(self.changesource.last_change is None)
self.assertTrue(self.changesource.last_change is None)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we use self.assertIsNone(self.changesource.last_change) here?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

yes, it is advised

@rutsky
Copy link
Copy Markdown
Member

rutsky commented Apr 1, 2016

Modern versions of unittest module have convenient methods like assertIn and assertIsNone, however I'm not sure is they were backported to Twisted's TestCase or is Twisted's TestCase has proper alternative.

@tardyp
Copy link
Copy Markdown
Member

tardyp commented Apr 1, 2016

twisted TestCase is a subclass of unittest TestCase..

else:
self.assert_(False, "No downloadFile command found")
else:
raise ValueError("No downloadFile command found")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do you move else here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

got confused. Also didn't realize there was the for...else: syntax.

self.assertEqual(b.results, SUCCESS)
self.assert_(('startStep', (self.workerforbuilder.worker.conn,), {})
in step.method_calls)
self.assertIn(('startStep', (self.workerforbuilder.worker.conn,), {}),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a good candidates for refactor in TestCase base classe (we use a tiny abstract class between unittest.TestCase and our tests, so we can implement our how missing assert* methods)...

self.assertMethodCalledWith(step, ('startStep', (self.workerforbuilder.worker.conn,), {}),

@gracinet
Copy link
Copy Markdown

gracinet commented Apr 2, 2016

By the way, assertEquals is deprecated in favor of assertEqual, if that matters
https://docs.python.org/2.7/library/unittest.html#deprecated-aliases

@tardyp
Copy link
Copy Markdown
Member

tardyp commented Apr 3, 2016

in this case, lets merge it.

@tardyp tardyp merged commit dea8036 into buildbot:master Apr 3, 2016
@ewongbb ewongbb deleted the ticket2521 branch April 4, 2016 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants