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 under the compiler directory to testament #16096

Merged
merged 4 commits into from
Nov 25, 2020

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Nov 22, 2020

tbtrees.nim:

  • BTree.entries is not exported, so I choose len instead.
  • Change some echo statements.

tnimblecmd.nim:

  • I use inlcude because some procs are not exported.
  • Original tests can't pass CI, so I change @["irc-#head", "another-0.1", "ab-0.1.3", "justone-1.0"] to @["irc-#head", "ab-0.1.3", "justone-1.0", "another-0.1"]

tprefixmatches.nim:

  • Change echo to doAssert

@ringabout ringabout marked this pull request as draft November 22, 2020 03:55
@ringabout ringabout marked this pull request as ready for review November 22, 2020 04:42
@ringabout
Copy link
Member Author

I think some when isMainModule cases are not tested by CI which is very bad.

@ringabout ringabout changed the title move tests to testament move tests under the compiler directory to testament Nov 22, 2020
@Araq
Copy link
Member

Araq commented Nov 23, 2020

I'm not sure the changes to parser.nim preserve the old semantics. We do nim c -r compiler/parser.nim to update the gammar that we extract from the parser.

@ringabout
Copy link
Member Author

ringabout commented Nov 23, 2020

I don't move the codes generate grammar files(and add a flag not to test it). I only move the codes check grammar.

The change:
image

@Araq
Copy link
Member

Araq commented Nov 24, 2020

I don't move the codes generate grammar files(and add a flag not to test it). I only move the codes check grammar.

Yeah but the old way was better, change the grammar and get a basic check for free.

@ringabout
Copy link
Member Author

parser changes are removed.

@Araq
Copy link
Member

Araq commented Nov 25, 2020

Merging it for now but I'm not convinced it's that a good idea. These tests now run on every commit but should only run when we change the related logic.

@Araq Araq merged commit 57bd645 into nim-lang:devel Nov 25, 2020
@ringabout
Copy link
Member Author

ringabout commented Nov 25, 2020

Yea, I agree.
only run when we change the related logic >(better than) These tests now run on every commit > not tested at all.

Before if using when isMainModule, only two places will be tested. Compiler/ is omitted.

  of "lib":
    testStdlib(r, "lib/pure/", options, cat)
    testStdlib(r, "lib/packages/docutils/", options, cat)

@timotheecour
Copy link
Member

this PR is a net improvement; those tests were never run before PR because of current testament logic, and even if testament would've been modified to run those, it's better to run them as separate module rather than isMainModule block for reasons already discussed.

only run when we change the related logic

this is always going to be brittle because of dependencies, a seemingly unrelated change in some dependency can break code. Unless there's a benchmark showing CI runtime is significantly impacted (which i doubt), it's best to just run those on each commit. In fact moving tests to separate files instead of isMainModule reduces test time because they can be joined via megatest.

@@ -0,0 +1,26 @@
include compiler/[nimblecmd]
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.

@xflywind instead of include how about, in a followup PR:

import compiler/[nimblecmd,options,lineinfos]
import std/[strtabs,sequtils]
# in nimblecmd.nim:
when defined(testing):
  export chosen, addPackage

(seems to work when i checked)

note that {.privateImport.} (#11865) would be much better (no global effect on other modules) but until it's merged the above snippet is still the better option, compared to include, which has many issues (as explained in #11865, in particular when a module will end up being duplicated)

Copy link
Member Author

Choose a reason for hiding this comment

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

I will check it.

Copy link
Member

Choose a reason for hiding this comment

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

The only reason why .privateImport doesn't have "many issues" is because almost nobody used it.

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.

The only reason why .privateImport doesn't have "many issues" is because almost nobody used it.

@Araq the reason nobody used it except myself is because the PR still hasn't been merged.
I've rebased it recently to address bitrot conflicts with devel so it should still be good to go / ready to address any further review comments.

besides having extensive tests in the PR, the safeguards are in place:
not only you can do --experimental:allowPrivateImport, but it can also be pushed/popped for more targetted use:

# in tnimblecmd
{.push experimental: "allowPrivateImport".}
from nimblecmd {.privateImport.} import chosen, addPackage
...
{.pop.}

=> other modules importing nimblecmd won't see chosen, addPackage, and nimblecmd won't be
duplicated, breaking modularity and other common issues with include

narimiran pushed a commit that referenced this pull request Nov 25, 2020
@ringabout ringabout mentioned this pull request Nov 25, 2020
1 task
ringabout added a commit to ringabout/Nim that referenced this pull request Nov 25, 2020
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
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.

3 participants