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

move tests to testament #16101

Merged
merged 4 commits into from
Nov 24, 2020
Merged

move tests to testament #16101

merged 4 commits into from
Nov 24, 2020

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
@@ -665,36 +665,6 @@ proc prevPermutation*[T](x: var openArray[T]): bool {.discardable.} =

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

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

@@ -52,7 +45,7 @@ suite "httpcore":
doAssert parseHeader("Accept: foo, bar, prologue") == (key: "Accept", value: @["foo", "bar", "prologue"])
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.

None yet

4 participants