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

Nimscript target for testament #17007

Open
metagn opened this issue Feb 11, 2021 · 2 comments
Open

Nimscript target for testament #17007

metagn opened this issue Feb 11, 2021 · 2 comments

Comments

@metagn
Copy link
Collaborator

metagn commented Feb 11, 2021

Summary

Testament should support a nims/nimscript target.

Description

Nim currently has a single nimscript test file, test_nimscript.nims, that individually gets run every time. Instead of this, we should be able to declare nimscript as a target for individual tests, so that we can test the VM as well as nimscript for modules that already get tested for other backends without copying the tests into test_nimscript.

There are already some tests that have to generate code twice to test in the VM (though I understand the compile time VM must also be tested for multiple backends), if it's thought not to matter which backend the VM runs in a test then nimscript would help. The imports in nimscript are also outdated, new modules get added pretty frequently and existing modules can silently become importable in nimscript.

The safest way to do this is either via copying the .nim file of the test into a .nims file, then running that test via nim e, then deleting it, though this will have a considerable load on IO. Maybe a way to allow files to have a .nim extension but still run them as nimscript would work.

Alternatives

test_nimscript.nims could individually import tests that need to be tested for nimscript, instead of copying the code directly. However import semantics could change behavior and this wouldn't be very comprehensive.

Additional Information

@juancarlospaco
Copy link
Collaborator

juancarlospaco commented Feb 11, 2021

Can be a symlink, Windows supports symlinks nowadays too, to skip copying the file.

@timotheecour
Copy link
Member

timotheecour commented Feb 12, 2021

if it's thought not to matter which backend the VM runs in a test

unfortunately, it matters, because when defined(js) affects VM too; some VM bugs are specific to js target for example.

So you testing nimscript doesn't replace testing vm (with our static: main() tests)

The imports in nimscript are also outdated, new modules get added pretty frequently and existing modules can silently become importable in nimscript

indeed. tests/test_nimscript.nims and tests/effects/tstrict_funcs_imports.nim (and some other tests) should use blacklists, not whitelists [1]

The safest way to do this is either via copying the .nim file of the test into a .nims file, then running that test via nim e, then deleting it, though this will have a considerable load on IO

I highly doubt this would have any meaningful performance impact. Every compilation already generates a bunch of c files; compilation costs always dominates. But this shouldn't be needed anyways, see below.

Can be a symlink, Windows supports symlinks nowadays too, to skip copying the file.

not needed (see below) and also doesn't work at least currently because of compiler/options.canonicalizePath which would resolve the symlink

ln -s main.nim mainb.nims
nim e mainb.nims
Error: not a NimScript file: main.nim

Maybe a way to allow files to have a .nim extension but still run them as nimscript would work.

  • we should do that IMO, lots of languages have such a feature, eg: clang -x c foo.cpp; in nim this would be:
nim e --ext:nims main.nim
nim c -r --ext:nim main.nims

proposal

so this is how I'd do it:

# tsugar.nim
discard """
  targets: "c js nimscript"
"""
template main =
  ... # all tests here
  block: # tests `foo`
    when not (defined(js) or defined(nimscript)): discard # xxx bug #xyz: not yet supported
    else: ...
main()
static: main()

#[
# or we can use a pattern for that:
# in stdtest/testutils:
template testMulti*(fn) =
  when defined(nimscript): fn()
  else:
    fn()
    static: fn()

# in here:
from stdtest/testutils import testMulti
testMulti(main)
]#

we could even have a vm target: targets: "c js vm nimscript" but it's unclear if this would be desirable; the vm needs to run for each backend (eg: c, js), as there are backend specific logic and bugs

BUG

  • nim r main.nims executes the same main.nims twice: once in nimscript, once in c backend

links

[1] Likewise, #16878 introduced a similar whitelist of modules tests/effects/tstrict_funcs_imports.nim as tests/test_nimscript.nims; whitelists are hard to maintain and keep up to date. Instead we need a blacklist, which should be easier to maintain (when modules are added, they'd be run automatically, and if needed, can be added to blacklist).

See how this can be done with findNimSrcFiles, refs https://github.com/nim-lang/fusion/pull/24/files (or see also kochdocs, which also has similar logic to generate files by walking filesystem on the fly)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants