Page MenuHomePhabricator

[lldb] Replace assertTrue(a == b, "msg") with assertEquals(a, b, "msg") in the test suite
ClosedPublic

Authored by teemperor on Wed, Feb 12, 3:09 AM.

Details

Summary

The error message from the construct assertTrue(a == b, "msg") are nearly always completely useless for actually debugging the issue.
This patch is just replacing this construct (and similar ones like assertTrue(a != b, ...) with the proper call to assertEqual or assertNotEquals.

This patch was mostly written by a shell script with some manual verification afterwards:

import sys

def sanitize_line(line):
  if line.strip().startswith("self.assertTrue(") and " == " in line:
    line = line.replace("self.assertTrue(", "self.assertEquals(")
    line = line.replace(" == ", ", ", 1)
  if line.strip().startswith("self.assertTrue(") and " != " in line:
    line = line.replace("self.assertTrue(", "self.assertNotEqual(")
    line = line.replace(" != ", ", ", 1)
  return line

for a in sys.argv[1:]:
  with open(a, "r") as f:
    lines = f.readlines()
  with open(a, "w") as f:
    for line in lines:
      f.write(sanitize_line(line))

Diff Detail

Event Timeline

teemperor created this revision.Wed, Feb 12, 3:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Feb 12, 3:09 AM
labath accepted this revision.Wed, Feb 12, 3:46 AM

I think this is a good idea. I have glanced over the patch, and I have found a couple of places where your script gets things wrong. They all involve doing boolean expressions in the assertion, which takes on a different meaning when you replace the comparison operator with a comma.

lldb/test/API/functionalities/process_group/TestChangeProcessGroup.py
70–71

This is not right. It should probably be two asserts.

lldb/test/API/macosx/thread-names/TestInterruptThreadNames.py
39–41

same here

lldb/test/API/tools/lldb-vscode/launch/TestVSCode_launch.py
147

An equivalent expression would be if output: assertEquals(len(output), 0), though I'm not sure why we'd need both checks tbh -- we should have a consistent way of reporting "no output"...

This revision is now accepted and ready to land.Wed, Feb 12, 3:46 AM
teemperor updated this revision to Diff 244133.Wed, Feb 12, 4:18 AM
  • Split up bool conditions (the one Pavel pointed out and one more).
teemperor edited the summary of this revision. (Show Details)Wed, Feb 12, 4:18 AM

LGTM. I don't know how complex your script is, but could you include it in the commit message or the script directory so that we can run this again in the future?

teemperor edited the summary of this revision. (Show Details)Thu, Feb 13, 4:34 AM
This revision was automatically updated to reflect the committed changes.

Either this change or D74243 broke the windows bot and I do suspect D74243 but have not confirmed it yet:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/13719

I've partially reverted this change (just for for TestBreakpointHitCount.py which is the one timing out on the Windows bot) in 16bf89267e5ac01d2b512ce784f23640a82de821.

Yup, looks like that was it, the bot is green again: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/13740

Thanks for taking care of it!