beetbox / beets

music library manager and MusicBrainz tagger
http://beets.io/
MIT License
12.92k stars 1.82k forks source link

Replace unittest assert methods by assert statement #5386

Closed snejus closed 3 months ago

snejus commented 3 months ago

... continuing the test suite migration to pytest (#5361), this PR replaces unittest method assertions with assert statement.

See the table below for a list of the replaced methods and their counts. This should help clarify the size of the pull request :laughing:.

To keep things simple for the review, I have replaced each method in its own commit. This way, things should be easy to follow if you step through the commits one by one. :slightly_smiling_face:

method count
assertEqual 1747
assertIn 200
assertTrue 149
assertIsNone 87
assertFalse 85
assertRaises 69
assertNotIn 64
assertNotEqual 39
assertIsNotNone 35
assertIsInstance 35
assertLessEqual 23
assertGreater 15
assertLess 14
assertGreaterEqual 14
assertAlmostEqual 9
assertRaisesRegex 4
assertCountEqual 2
assertListEqual 1

:exclamation: Note that this only replaces methods provided by unittest by default. Custom assertion method replacements (like assertExists will be addressed in a separate PR).

github-actions[bot] commented 3 months ago

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

Serene-Arc commented 3 months ago

For a PR like this, if it's done with like a grep and the tests still pass, I don't think it requires more review than a sanity check. Is that what you did, or did you go through by hand?

snejus commented 3 months ago

For a PR like this, if it's done with like a grep and the tests still pass, I don't think it requires more review than a sanity check. Is that what you did, or did you go through by hand?

Most of it was done using a several sed search and replace patterns (see below). Then I later discovered a tool unittest2pytest which I used to double-check my replacements.

I did review the diff manually though since it failed to replace some of the more complicated assertions where I saw the tests fail.

oneline_python() {
  # args: <python-file> ...
  # info: _unformat given .py files: remove newlines from each statement
  sed -zr '
    s/(^|\n) *# [^\n]*//g
    s/  # [^\n]*//g
    s/,?\n\s*([]}).]+)/\1/g
    s/\n\s+(\b(and|or)\b|==)/ \1/g
    s/,\s*\n\s+/, /g
    s/"\s*\n\s+(%|f['\''"])/" \1/g
    s/([[{(])\s*\n\s+/\1/g
    s/(["'\''])\n\s*(["'\''+])/\1 \2/g
    s/\n\s*( \+)/\1/g
    s/(\[[^]\n]+)\n\s*( for)/\1\2/g
  ' $@ |
    tr '\000' '\n'
}

gdfd() {
  # args: <git-args> <pattern>
  # info: show hunks that match the _<pattern>
  local argidx
  argidx=${@[(rI)[^-]*]}

  git diff -U1 ${@[1, $(( argidx - 1 ))]} |
    grepdiff --output-matching=hunk --only-match=mod ${@[$argidx,-1]} | {
      if [[ -t 1 ]]; then
        diff-so-fancy
      else
        cat
      fi
    }
}

unittest2pytest() {
  # args: <python-file> ...
  # info: convert _unittest assertions to _assert statements
  local file
  local contents
  # black -C --line-length=100000 $@
  for file in $@; do
    oneline=$(oneline_python $file)
    print -R $oneline > $file
    gdfd -U0 $file '  # ' | git apply -R --unidiff-zero --allow-empty
    sed -r '
      /def |assert(Raises|Is(Ins|File|Dir)|Counts|(Not)?(Exist|InResult)|EqualTimes|AlbumImport|Perms|Contains|(No)?File|Item|Excludes|Negation|LyricsCon)/!{
        /[a-z]+.assert[A-Z]/{
          s/\(/ /
          s/\)(  #|$)/\1/
          s/msg=//
          s/[a-z._]*assertTrue (([^()]|\([^)]*\))+)/assert \1/
          s/[a-z._]*assertFalse (([^()]|\([^)]*\))+)/assert not \1/
          s/[a-z._]*assertIsNone (.*)(, |$)/assert \1 is None/
          s/[a-z._]*assertIsNotNone (.*)(, |$)/assert \1 is not None/
          s/[a-z._]*assert(List)?Equal (([^,]|\([^)]*\)|\{[^}]*\}|\[[^\]]*\])+), (#[^@]+@ )?(([^,]|\([^)]*\)|\[[^]]*\])+)/assert \2 == \5/
          s/[a-z._]*assertNotIn ([^,]+), (.*)$/assert \1 not in \2/
          s/[a-z._]*assertIn ([^,]+), (.*)$/assert \1 in \2/
          s/[a-z._]*assertLess ([^,]+), (([^()]|\([^)]*\))+)$/assert \1 < \2/
          s/[a-z._]*assertLessEqual ([^,]+), (([^()]|\([^)]*\))+)$/assert \1 <= \2/
          s/[a-z._]*assertGreaterEqual ([^,]+), (.*)$/assert \1 >= \2/
          s/[a-z._]*assertGreater ([^,]+), (.*)$/assert \1 > \2/
          s/[a-z._]*assertNotEqual ([^,]+), (.*)$/assert \1 != \2/
          # s/[a-z._]*assertExists ([^)]+)/assert Path(\1).exists()/
          # s/[a-z._]*assertNotExists ([^)]+)/assert not Path(\1).exists()/
          /[a-z._]*assertAlmostEqual ([^,]+), ([^,)]+(, (places|delta)=[0-9]+)?)/{
            s//assert \1 == pytest.approx(\2)/
            s/places=/rel=1e-/
            s/delta=/abs=/
          }
          # s/[a-z._]*assertEqualTimes ([^,]+), ([^,]+)/assert \1 == pytest.approx(\2, rel=1e-4)/
          # s/[a-z._]*assertInResult ([^,]+), (.*)/assert \1.id in {i.id for i in \2}/
          # s/[a-z._]*assertNotInResult ([^,]+), (.*)/assert \1.id not in {i.id for i in \2}/
        }
      }
      s/[a-z._]*assertRaisesRegex(\([^,]+, )/pytest.raises\1match=/
      /[a-z._]*assertRaises/{
        s//pytest.raises/
        s/^(( +)([a-z][^,]+)), ([^,]+)(, ([^)]+))?\)$/\2with \3):\n\2    \4(\6)/
      }
      s/[a-z._]*assertIsInstance/assert isinstance/
    ' -i $file
  done
  poe format $@
}
Serene-Arc commented 3 months ago

In that case I'll give all of it a quick lookover.