Replace unittest assert methods by assert statement

[snejus]

... 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 😆.

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. 🙂

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

❗ 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]

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]

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]

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]

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

[bal-e]

Great job, @snejus!

[bal-e]

If the exception message should be exactly this string, can you add ^$ around it? Apparently this argument is passed to re.search() which will check for a substring rather than an exact match.

[Serene-Arc]

Yeah, this is the only thing that I found. Everything else is equivalent to the previous tests as far as I can see.

[snejus]

Do you reckon we should be asserting the entire exception message?

I tend to assert only the relevant part in the exception message, for example

from contextlib import ExitStack as does_not_raise

import pytest
from django.core.management import call_command
from django.core.management.base import CommandError


def error(message_pattern: str):
    return pytest.raises(CommandError, match=message_pattern)


@pytest.mark.parametrize(
    "ids, expected_error",
    (
        ([], error(r"required: ids"), 0),
        ([0, 1], error(r".*do not exist"), 0),
        ([1, 9], error(r"already have request"), 0),
        ([1, 2], does_not_raise(), 2),
    ),
)
def test_create_leave_requests_for_leave(ids, expected_error):
    with expected_error:
        call_command("my_command", *map(str, ids))

this allows the rest of the wording in the message to change. For example, if someone corrects the spelling in the rest of the message, tests won't fail!

[bal-e]

If there's a spelling mistake in the rest of the message, it should be caught by the tests. If you know what the entire error message should be, and it's at least consistent across platforms, then I think it is best to check for it.

(Localization of user-facing error messages is a different story, one I don't think beets is in any position to think about right now.)

[snejus]

Localisation would indeed be an interesting issue to tackle, but like you said beets it's in no position to achieve it soon.

The situation I am talking about is something similar to my changes in autobpm, for example:

If we had two functionally equivalent assertions

assert error == "LibsndfileError: failed to load /a/b/c.mp3"
assert "failed to load" in error.lower()

Even if my change kept the relevant information, the first assertion would have started failing, while the second one would have passed.

[snejus]

I'm merging this in but keeping this thread open for the discussion 😄