Skip to content

Conversation

@ringabout
Copy link
Member

@ringabout ringabout commented Nov 23, 2020

  • change two or three unittest style to doAssert
  • use include because some symbols are not exported
  • disable testing random module

@Araq Araq merged commit cbc793b into nim-lang:devel Nov 24, 2020
narimiran pushed a commit that referenced this pull request Nov 25, 2020
* move tests to testament

* minor

* fix random

* disable test random

(cherry picked from commit cbc793b)
ringabout added a commit to ringabout/Nim that referenced this pull request Nov 25, 2020
* move tests to testament

* minor

* fix random

* disable test random

result = true

when isMainModule:
Copy link
Member

Choose a reason for hiding this comment

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

@xflywind
after PR we now have both tests/stdlib/ttables.nim and tests/collections/ttables.nim
(but it was indeed better to delay fixing this to a separate PR so this one stays a simple move operation)

these should be merged into a single one

@timotheecour
Copy link
Member

to review this PR, you really need git diff --color-moved=blocks --color-moved-ws=ignore-all-space as recommended here #13162 otherwise github / diff without options won't tell you what actually changed vs what was moved...

let foo = parseUri("http://example.com") / "/baz"
doAssert foo.path == "/baz"

# bug found on stream 13/10/17
Copy link
Member

@timotheecour timotheecour Nov 25, 2020

Choose a reason for hiding this comment

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

for next time, prefer this style in 1 line:

block: # bug found on stream 13/10/17
  • more consistent with block logic
  • more similar to block: label
  • more similar to test "VM and runtime should in unittests
  • more common and shorter

doAssert parseHeader("Accept: foo, bar, prologue, starlight") == (key: "Accept", value: @["foo", "bar", "prologue", "starlight"])

test "add empty sequence to HTTP headers":
block add_empty_sequence_to_HTTP_headers:
Copy link
Member

Choose a reason for hiding this comment

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

prefer this style:

block: "add empty sequence to HTTP headers"

Copy link
Member

Choose a reason for hiding this comment

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

Why?

mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
* move tests to testament

* minor

* fix random

* disable test random
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
* move tests to testament

* minor

* fix random

* disable test random
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants