Opened 6 years ago

Closed 3 years ago

#2521 closed enhancement (fixed)

Replace deprecated method unittest.TestCase.assert_() with newer assertEqual/NotEqual

Reported by: rutsky Owned by: ewong
Priority: major Milestone: undecided
Version: 0.8.7p1 Keywords: tests simple
Cc: rutsky.vladimir@…

Description

Some of tests use deprecated unittest.TestCase.assert_() method, like in test_changes_mail_CVSMaildirSource.py:

    self.assert_(chdict['revision'] == '2010-08-11 04:56:44')

When such test fails developer only know, that chdict['revision'] is not '2010-08-11 04:56:44', but he doesn't know which value it actually has --- that is often required for proper problem analysis.

Use for modern assertEqual(), assertNotEqual() solves this problem.

Change History (10)

comment:1 Changed 6 years ago by rutsky

Also, it's tempting to use other modern methods, such as assertIsNone() and assertIn(), that was introduced in Python 2.7's unittest module.

Maybe use separate unittest2 module (backport of unittest for older Python) instead provided with Python unittest?

comment:2 Changed 6 years ago by rutsky

I made pull request that replace assert_() in master/buildbot/test/unit/test_changes_mail_CVSMaildirSource.py: https://github.com/buildbot/buildbot/pull/781

comment:3 Changed 6 years ago by rutsky

Only now I noticed, that Buildbot mostly uses Twisted's unittest module which already extends unittest in some way.

So adding unittest2 as additional dependency is bad idea --- better to monkey-patch Twisted implementation if it doesn't have any required feature.

Also I found that Tom Prince recently reviewed extending of Twisted's TestCase.

comment:4 Changed 6 years ago by dustin

That pull request is merged. The lack of "modern" assertions is annoying, but not fatal. Is there anything else to do in this bug?

comment:5 Changed 6 years ago by rutsky

I would like to replace other use of assert_() in Buildbot --- my pull request fixed this issue only in test_changes_mail_CVSMaildirSource.py. Later I'll make pull requests that replaces assert_() in other places (there are two or five files more as I remember).

comment:6 Changed 6 years ago by dustin

Ah, great. I assume a simple replacement with failUnless would be a good start, followed by using assertEqual or the like where the assertion condition is a comparison operator.

comment:7 Changed 4 years ago by rutsky

  • Keywords simple added

comment:8 Changed 4 years ago by rutsky

Looks like I never made PR that finally fixes this issue:

$ git grep 'assert_('
master/buildbot/test/unit/test_changes_p4poller.py:        self.assert_(self.changesource.last_change is None)
master/buildbot/test/unit/test_process_build.py:        self.assert_(('startStep', (self.workerforbuilder.worker.conn,), {})
master/buildbot/test/unit/test_process_build.py:        self.assert_(('interrupt', ('stop it',), {}) in step.method_calls)
master/buildbot/test/unit/test_process_build.py:            self.assert_(('interrupt', ('stop it',), {}) in step1.method_calls)
master/buildbot/test/unit/test_process_build.py:            self.assert_(step2Started[0])
master/buildbot/test/unit/test_process_build.py:        self.assert_(('startStep', (self.workerforbuilder.worker.conn,), {})
master/buildbot/test/unit/test_process_build.py:        self.assert_(b.currentStep is None)
master/buildbot/test/unit/test_process_build.py:        self.assert_(b._acquiringLock is not None)
master/buildbot/test/unit/test_process_build.py:        self.assert_(('startStep', (self.workerforbuilder.worker.conn,), {})
master/buildbot/test/unit/test_process_build.py:        self.assert_(b.currentStep is None)
master/buildbot/test/unit/test_process_build.py:        self.assert_(('interrupt', ('stop it',), {}) not in step.method_calls)
master/buildbot/test/unit/test_process_build.py:        self.assert_(('startStep', (self.workerforbuilder.worker.conn,), {})
master/buildbot/test/unit/test_process_build.py:        self.assert_(b.currentStep is None)
master/buildbot/test/unit/test_process_build.py:        self.assert_(('interrupt', ('stop it',), {}) not in step.method_calls)
master/buildbot/test/unit/test_process_build.py:            self.assert_(b.currentStep is step)
master/buildbot/test/unit/test_process_metrics.py:        self.assert_(metrics._get_rss() > 0)
master/buildbot/test/unit/test_process_metrics.py:        self.assert_(observer.log_task)
master/buildbot/test/unit/test_process_metrics.py:        self.assert_(observer.periodic_task)
master/buildbot/test/unit/test_process_metrics.py:        self.assert_(observer.log_task)
master/buildbot/test/unit/test_process_metrics.py:        self.assert_(observer.periodic_task)
master/buildbot/test/unit/test_process_properties.py:        self.assert_('do-tests' in self.props)
master/buildbot/test/unit/test_steps_transfer.py:            self.assert_(False, "No downloadFile command found")

It should easy to fix.

comment:9 Changed 4 years ago by ewong

  • Owner set to ewong
  • Status changed from new to assigned

comment:10 Changed 3 years ago by rutsky

  • Resolution set to fixed
  • Status changed from assigned to closed

Looks like this was fixed some time ago. Plus this PR fixes other deprecations.

This is all in master, *not* in eight branch.

Note: See TracTickets for help on using tickets.