Assert statements will now raise a check50.Failure by default

[ivanharvard]

Related to #228.

Using assert will now no longer raise Python's standard AssertionError.

When check50 is ran in a CLI, it will make a temporary copy of the checks file, rewrite all assertions, and then run the checks on that temporary, modified file.

For instance, a check written as so:

@check50.check(compiles)
def test1():
    """input of 'abc'"""
    output = check50.run('./test').stdout()
    assert output == 'abc'

will be rewritten internally as:

from check50.assertions import check50_assert        # top of file

@check50.check(compiles)
def test1():
    """input of 'abc'"""
    output = check50.run('./test').stdout()
    check50_assert(output == 'abc', "output == 'abc'")

If output == 'abc' evaluates to False, a check50.Failure exception is raised with the message: Assertion failure: output == 'abc.

Custom messages are also supported. For instance,

assert output == 'abc', "Expected output to be 'abc'"

will now raise check50.Failure with the message Expected output to be 'abc'.

Custom exceptions are also supported. For instance,

assert output == 'abc', check50.Mismatch('abc', output)

will now raise check50.Mismatch('abc', output).

If the user adds a valid Python object but it is neither an exception or string, the additional argument is silently ignored and a check50.Failure is raised with the default message. For instance

assert output == 'abc', 123

will now raise check50.Failure with the message Assertion failure: output == 'abc.

[ivanharvard]

Added conditional inferencing on basic binary conditions.

Currently, == and in are the only operators supported. For instance:

assert x == y

will now raise check50.Mismatch(x, y), and

assert x in y

will now raise check50.Missing(x, y).

This inferencing can be overridden by simply including the correct exception (or message) to raise:

assert x in y, check50.Mismatch(x, y)

will still raise check50.Mismatch(x, y).

More complex conditions, such as:

assert (x in y) and (a == b)

will still raise a check50.Failure exception with the message "assertion failed: (x in y) and (a == b)"

Also added an additional note in the docstring for check50_assert that clarifies usage on non-check50 exceptions:

Exceptions from the check50 library are preferred, since they will be
handled gracefully and integrated into the check output. Native Python
exceptions are technically supported, but check50 will immediately
terminate on the user's end if the assertion fails.

[Jelleas]

Nice stuff! Here are my thoughts after trying some checks:

Rephrasing "assertion failed:"

This would require some explaining for students in early phases, maybe "Expected" or "Checking" is easier to grasp.

Difference in behavior

There is a small difference in behavior between the default check50.Failure and the more specific errors. For the default Failure you get the message "assertion failed" followed by the literal comparison output < "foo". Here variable names are exposed to the student in the message:

assertion failed: output < 'foo'

This can be useful, but annoying too as you'd want to carefully pick your variable names because they might just end up as feedback :)

Whereas the specific errors don't show the variable names, but the values instead:

assert "!" in output

gives:

Did not find "!" in "hello"

Again useful, but annoying too as most of the times you'd need to signal in some way where that "actual" value is coming from. Although for shorter and simpler checks this is often implicit from the check description.

Now in both cases you'd need to be mindful of two different things: either pick your variable names wisely, or remember to mention where "actual" is coming from.

Unsure what the best course of action is. I see both behaviors having their application, you might want to signal the variable output to the student, or in other cases you'd want to show what their output actually is. The latter being the most common I think.

That said, you can overwrite it both ways:

assert output < 'foo', f"expecting output < 'foo', where your program's output is '{output}'"

and

assert "!" in output, check50.Missing("!", output, 'assertion failed: "!" in output')

So in that sense the control for a check writer to show what they want is already there. It just seems odd that the default behavior is different.

Maybe do both all the time? Rewrite:

assert output < 'foo'

to

assert check50_assert(output < 'foo', "output < 'foo'", help='where output is '{output}'"')

and in the case of assert "!" in output add the literal comparison as help.

This is in line with pytest, just if there is more than one variable the help line could get lengthy. But again, why would you reasonably have more than two to three variables there.

Missing import

Missing import shutil in __main__.py

str all the things

raise Mismatch(left, right) and raise Missing(left, right) require both left and right to be strings. This causes non str asserts to raise an error:

assert 1 == 2

=>

TypeError: object of type 'int' has no len()
      File "/Users/jelle/Projects/check50/check50/runner.py", line 146, in wrapper
    state = check(*args)
      File "/var/folders/wy/zfwhdcrj5xq0317j273s1pv80000gn/T/tmpbrgl_bus.py", line 7, in test
    check50_assert(1 == 2, '1 == 2', None, cond_type='eq', left=1, right=2)
      File "/Users/jelle/Projects/check50/check50/assertions/runtime.py", line 61, in check50_assert
    raise Mismatch(left, right)
      File "/Users/jelle/Projects/check50/check50/_api.py", line 458, in __init__
    expected, actual = _truncate(expected, actual), _truncate(actual, expected)
      File "/Users/jelle/Projects/check50/check50/_api.py", line 513, in _truncate
    limit = min(len(s), len(other))

Should probably change to raise Mismatch(str(left), str(right))

[ivanharvard]

Thanks for all the feedback! I've implemented most of these changes:

• Renamed assertion failed: to check did not pass:
• Inferences on Missing and Mismatch will now always display the src. For instance,

assert "1" == '2'

will raise check50.Mismatch and display this message:

    expected: "2"
    actual:   "1"
    checked: '1' == '2'

• Inferences on Missing and Mismatch will display additional variable context if available. For instance,

num1 = '1'
num2 = '2'
assert num1 in num2

will raise check50.Missing and display this message:

    Did not find "1" in "2"
    checked: num1 in num2
    where num2 = '2', num1 = '1'

Mixing of variables and literals will also work.

• Inferences on Mismatch has had its order switched. Before, the expected value had to lie on the left side of the equality sign. Now, the expected value lies on the right.

num1 = '1'
num2 = '2'
assert num1 == num2

will raise check50.Mismatch and display this message:

    expected: "2"
    actual:   "1"
    checked: num1 == num2
    where num1 = '1', num2 = '2'

As opposed to:

    expected: "1"
    actual:   "2"
    checked: num1 == num2
    where num1 = '1', num2 = '2'

• By default, the src and the variable context will also be printed. For instance,

num1 = '1'
assert num1 > '2'

will raise check50.Failure and display this message:

    check did not pass: num1 > '2'
    where num1 = '1'

• shutil error fixed in latest commit
• Non-str arguments fixed in check50.Mismatch from v4.0.0-dev fails on non-string inputs #365

[Jelleas]

Nice! Good call on the actual expected ordering :)

Here are some things I ran into during some quick testing:

In case of function calls or method calls it is now showing the value of the variables (a function / object respectively) and not the return value. See the "where" line below.

@check50.check()
def foo():
    """foo"""
    assert check50.run("pwd").stdout() == "foo"

@check50.check()
def bar():
    """bar"""
    def get_output():
        return "bar"
    assert get_output() == "foo"

$ check50 --dev ~/foo
Checking...
Results for /Users/jelle/foo generated by check50 v3.3.7
:( foo
    expected: "foo"
    actual:   "/private/v..."
    checked: check50.run('pwd').stdout() == 'foo'
    where check50 = <module 'check50' from '/Users/jelle/Projects/check50/check50/__init__.py'>
    running pwd...
    running pwd...
:( bar
    expected: "foo"
    actual:   "bar"
    checked: get_output() == 'foo'
    where get_output = <function bar.<locals>.get_output at 0x105aeba30>

Any function call in the assert is evaluated twice through the rewrite. See check50_assert(c.stdout() == 'foo', "c.stdout() == 'foo'", None, cond_type='eq', left=c.stdout(), right='foo', context={'c': c}) below.

@check50.check()
def baz():
    """baz"""
    c = check50.run("pwd")
    out = c.stdout()
    assert out == 'foo'

@check50.check()
def qux():
    """qux"""
    c = check50.run("pwd")
    assert c.stdout() == 'foo'

$ check50 --dev ~/foo
Checking...
Results for /Users/jelle/foo generated by check50 v3.3.7
:( baz
    expected: "foo"
    actual:   "/private/v..."
    checked: out == 'foo'
    where out = '/private/var/folders/wy/zfwhdcrj5xq0317j273s1pv80000gn/T/tmpdjxzjvee/baz\n'
    running pwd...
:| qux
    check50 ran into an error while running checks!
    ValueError: I/O operation on closed file.
      File "/Users/jelle/Projects/check50/check50/runner.py", line 146, in wrapper
    state = check(*args)
      File "/var/folders/wy/zfwhdcrj5xq0317j273s1pv80000gn/T/tmphlqpjetv.py", line 28, in qux
    check50_assert(c.stdout() == 'foo', "c.stdout() == 'foo'", None, cond_type='eq', left=c.stdout(), right='foo', context={'c': c})
      File "/Users/jelle/Projects/check50/check50/_api.py", line 254, in stdout
    self._wait(timeout)
      File "/Users/jelle/Projects/check50/check50/_api.py", line 365, in _wait
    self.process.expect(EOF, timeout=timeout)
      File "/Users/jelle/.virtualenvs/check50/lib/python3.10/site-packages/pexpect/spawnbase.py", line 343, in expect
    return self.expect_list(compiled_pattern_list,
      File "/Users/jelle/.virtualenvs/check50/lib/python3.10/site-packages/pexpect/spawnbase.py", line 372, in expect_list
    return exp.expect_loop(timeout)
      File "/Users/jelle/.virtualenvs/check50/lib/python3.10/site-packages/pexpect/expect.py", line 169, in expect_loop
    incoming = spawn.read_nonblocking(spawn.maxread, timeout)
      File "/Users/jelle/.virtualenvs/check50/lib/python3.10/site-packages/pexpect/pty_spawn.py", line 443, in read_nonblocking
    raise ValueError('I/O operation on closed file.')
    running pwd...

Where clauses are not formatted on a new line in the web-view:

[ivanharvard]

Got it! I've fixed some of these bugs today. Definitely was a challenge :)

Having this check:

@check50.check()
def foo():
    """foo"""
    assert check50.run("pwd").stdout() == "foo"

@check50.check()
def bar():
    """bar"""
    def get_output():
        return "bar"
    assert get_output() == "foo"

will result in this error message

:( foo
    expected: "foo"
    actual:   "/tmp/tmpv_..."
    checked: check50.run('pwd').stdout() == 'foo'
    where check50.run('pwd').stdout() = '/tmp/tmpv_284mm1/foo\n', 'foo' = 'foo'
    running pwd...
    running pwd...
:( bar
    expected: "foo"
    actual:   "bar"
    checked: get_output() == 'foo'
    where get_output() = 'bar', 'foo' = 'foo'

Essentially, during the rewrite stage, I parsed the conditional, and if I found a function, I would reconstruct it as a string from the ground up, and awaited evaluation. Then, I used eval on that string during the runtime of check50_assert (I was concerned about the use of eval, as you probably are, but I realized it was no more unsafe than .run()ing or importing a student's file, which is where the malicious code would come from).

Now, there's an issue with the current iteration of this assertion rewrite, and it has to do with what you've mentioned already:

Any function call in the assert is evaluated twice through the rewrite.

This was originally caused by me evaluating and injecting the left side and right side of the conditional (if it existed) into check50_assert). I've updated it that so left and right variables are now passed as strings, and evaluated alongside the other functions or variables included in the assertion statement.

Unfortunately, this merely delays the problem, and there's still two executions of the same function. I haven't figured out how to fix this yet, but I think a viable solution would be wrapping every function contained in the assertion conditional in some kind of memoization function that stores its results and that of its arguments (if the arguments are function calls as well). I think this could avoid us having to use eval, too.

Something like:

assert foo(bar, qux())

to

check50_assert(memo(foo(bar, memo(qux()))), ...)

What do you think? Looks a bit messy, but it shouldn't be visible to the check writer anyways. I'll probably have to implement this tomorrow.

The last issue is something I have to look at where the HTML/Jinja does not interpret new lines as line breaks within the help message that was passed, but I believe that should be a much easier fix. I'll either look into that tomorrow as well or open a ticket soon. Try it out if you'd like, and if you find any more bugs for me to patch, I'll see what I can do to fix them. Thanks again!

[Jelleas]

I was writing this while a new commit popped up, unsure if still relevant:

From a distant point of view, wouldn't it make sense to write the visit methods for all the operator Nodes (UnaryOp, BinOp, BoolOp, Compare). Then you could take the left and/or right side and wrap those sides (the whole expression) in some sort of memo / log function. Might be easier / less code that way.

Edit: ^ but you would be dealing with nested expressions and that might not be simple to solve.

On some futher thought: in case of testing through asserts in the context of check50 it's probably not as much about the individual names, but rather about operators being applied to expressions and their results. For instance, some expressions as complex as check50.run("./mario").stdout() is just one conceptual thing. In a professional setting you might want to know what check50 evaluates to, and .run, and stdout..., but in the case of check50 it's likely only the entire expression that is ever relevant.

Small catch on memoization, you'd probably need to use ids in the memo function. Here's why:

from random import random
import check50

check50.check()
def foo():
    """foo"""
    assert random() == random() # :)

Perhaps:

assert check50_assert(memo("random()", 0) == memo("random()", 1), ...)

[ivanharvard]

Finally fixed it!

I ended up not passing in the bare conditional at all and went for creating a new, evaluatable src string that could determine whether the conditional was true or not. I did this by setting all the variables and funcs as dummy variables, then replacing every instance of each function or variable with its respective dummy variable. Afterwards, I used a context dictionary to define what each dummy variable's value was. See substitute_expressions for a bit more detail:

Rewrites src by replacing each key in context with a placeholder variable name,
and builds a new context dict where those names map to pre-evaluated values.

For instance, given a src:

check50.run('pwd').stdout() == actual

it will create a new eval_src as

__expr0 == __expr1

and use the given context to define these variables:

eval_context = {
    '__expr0': context['check50.run('pwd').stdout()'],
    '__expr1': context['actual']
}

Then, running eval on eval_src given eval_context correctly evaluates the original src.

Now, running these checks:

@check50.check()
def foo():
    """foo"""
    assert check50.run("pwd").stdout() == "foo"

@check50.check()
def bar():
    """bar"""
    def get_foo():
        return "foo"
    def get_output(arg):
        return "bar"
    assert get_output(get_foo()) == "foo"

results in this:

[ivanharvard]

Note to future self: tests failed on commit because the file in check50/tests/checks/internal_directories/init.py has assert statements. Rerun the tests when the new Config class is merged with the 4.0.0-dev branch, pull it, and add to it a new flag to enable or disable assertion statement rewrites. This should probably fix the issue.