Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash and new features in unittest #4639

Closed
wants to merge 4 commits into from

Conversation

jdmansour
Copy link

The following unit test crashes currently:

import unittest

suite "ShowsBug":
  test "Compare with nil variable":
    let str: string = nil
    check str == "hello"
Traceback (most recent call last)
unittest.nim(267)        test_bug
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

The reason is that the unittest module just checks wheter string($str) compiles, and then prints out $str even if str is nil. I've added a check for nil and it doesn't crash anymore.

Maybe long term it should use something like repr instead of $, which handles nil and also adds quotation marks. I didn't use it here because it also prepends the memory address, which I didn't want in the test output.

@jdmansour
Copy link
Author

jdmansour commented Aug 22, 2016

I've added a few simple improvements to the unittest module. Seems GitHub added them to my bugfix pull request, hope that is OK.

  • Print out values of nested dot expressions on failure:
test_feature.nim(17,23): Check failed: c.color.name == "red"
c.color.name was blue
  • Same with bracket expressions
test_feature.nim(18,26): Check failed: c.passengers[0] == "Anna"
c.passengers[0] was Anne
  • Suppress constants (enum members) in output, like the last line here:
test_feature.nim(22,15): Check failed: meth == httpPOST
meth was httpGET
httpPOST was httpPOST
  • Allow let inside of check blocks. I found this really nice if you tested a list of invariants, and want to factor out a variable in the middle.

I've tried to find any unintended side effects, but so far these changes seem OK and backwards-compatible.

Here is a test case (the first two tests fail on purpose, the last doesn't compile without patch):

import unittest

type
  Car = object
    color: Color
    passengers: seq[string] not nil

  Color = object
    name: string not nil

  HttpMethod = enum
    httpGET, httpPOST

suite "ShowsFeatures":
  test "Shows fields on failure":
    let c = Car(color:Color(name:"blue"), passengers: @["Anne", "Bob"])
    check c.color.name == "red"
    check c.passengers[0] == "Anna"

  test "Supresses constants on failure":
    let meth = httpGET
    check meth == httpPOST

  test "Allow let in check":
    let x = 3
    check:
      x == 3
      let square = x*x
      square == 9

Here is the output with patch

test_feature.nim(18,23): Check failed: c.color.name == "red"
c.color.name was blue
test_feature.nim(19,26): Check failed: c.passengers[0] == "Anna"
c.passengers[0] was Anne
[FAILED] Shows fields
test_feature.nim(23,15): Check failed: meth == httpPOST
meth was httpGET
[FAILED] Supresses constants
[OK] Allow let in check

@jdmansour jdmansour changed the title Fix crash when string is nil in failing unittest Fix crash and new features in unittest Aug 22, 2016
@@ -270,6 +284,10 @@ macro check*(conditions: untyped): untyped =
var paramAst = exp[i]
if exp[i].kind == nnkIdent:
argsPrintOuts.add getAst(print(argStr, paramAst))
if exp[i].kind == nnkDotExpr:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a case exp[i] here please.

@Araq
Copy link
Member

Araq commented Aug 22, 2016

Please also add the test case. The testing framework is not really documented, but you can look at over 500 examples to figure it out. :-)

@jdmansour
Copy link
Author

Thanks for the review. I also just found out that there is a builtin isNil, so I should probably change that.

Once I figure out how to write a test that passes if it actually fails but doesn't crash, I'll add the test.

@dom96
Copy link
Contributor

dom96 commented Sep 15, 2016

This is really nice, thank you for your contribution!

For the future, it's helpful to create a new branch for each PR. That way you can work on other things in another branch, and not worry about an older PR including your new commits.

@FedericoCeratto FedericoCeratto mentioned this pull request Jan 10, 2017
16 tasks
@andreaferretti
Copy link
Collaborator

@jdmansour Did you have the time to fix the issues here?

@Araq
Copy link
Member

Araq commented Sep 15, 2017

No feedback. Closing.

@Araq Araq closed this Sep 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants