Saem nimsuggest re-enable tests #16401

saem · 2020-12-19T05:31:01Z

Bolster test suite in order to serve catch regressions:

re-enabled tests:
- tchk1.nim: check (chk) via nimsuggest works at end of file
- tdot4.nim: prioritize already used completion
- tinclude.nim: definition lookup (def) with includes
- tstrutils.nim -> tdef2.nim: test template definition lookup (def)
- tsug_regression.nim: regression test for nimsuggest #52
- ttemplate_highlight.nim: per the file name
- twithin_macro_prefix.nim: suggest within a macro with a prefix
created/extracted fixture modules into a sub-directory for tests to use:
- less likely to break from stdlib changes
- tests run faster
misc:
- left a few todos for follow-up PRs
- twithin_macro.nim was not enabled as it doesn't seem to provide a good signal

saem · 2020-12-19T05:36:42Z

Notes:

made some edits to the actual code over which nimsuggest was being tested
highlight is still disabled for EPC to avoid changing actual nimsuggest behaviour
added detailed notes per test including leaving open questions

ToDo:

~~it looks like tester doesn't test v1 of the protocol so that should be segregated~~ will do this later/if required
~~test suite is slow -- see if these can be made more unit test like skipping the full frontend~~ done

saem · 2020-12-19T05:39:59Z

Type of feedback I'm especially interested in:

looking for guidance overall -- seem like a reasonable start before venturing into actually changing nimsuggest?
specific feedback on any of the changes I made to get things working again
answers for any of the open questions in the various tests

timotheecour · 2020-12-19T20:00:48Z

nimsuggest/tests/tdot4.nim

 $nimsuggest --tester --maxresults:2 $file
 >sug $1
 sug;;skProc;;tdot4.main;;proc (inp: string): string;;$file;;10;;5;;"";;100;;None
-sug;;skProc;;strutils.replace;;proc (s: string, sub: string, by: string): string{.noSideEffect, gcsafe, locks: 0.};;$lib/pure/strutils.nim;;1506;;5;;"Replaces `sub` in `s` by the string `by`.";;100;;None
+sug;;skFunc;;strutils.replace;;proc (s: string, sub: string, by: string): string{.noSideEffect, gcsafe, locks: 0.};;*/lib/pure/strutils.nim;;*;;5;;"Replaces `sub` in `s` by the string `by`.*";;100;;None


please add dedicated module(s) only imported by nimsuggest tests:
nimsuggest/tests/mimported.nim
otherwise we'll end up with nimsuggest tests always failing everytime a change is made to those modules in stdlib.

(as you've experienced in your PR)

nimsuggest tests should only rely on its own support files nimsuggest/tests/m*.nim

If for some reason we really want to try nimsuggest on a real codebase (eg compiler/nim.nim), then at least the test should be made more robust to code changes, using for example:

let (outp, exitCode) = execShellCmdEx("nimsuggest --tester ... compiler/nim.nim") doAssert exitCode == 0 doAssert "somethingExpected" in outp # etc, makes it more robust to line changes

but I'm not sure that's needed, we can just ensure nimsuggest/tests/mimported.nim is comprehensive enough to add every pattern that needs testing

Awesome, thanks for the detailed feedback, @timotheecour.

I was wondering about how to isolate for those and my head jumped to using metamorphic testing, but it's likely far simpler to do it this wya. The only issue is that I believe no matter what system.nim will always get in there. That might also be a good thing.

Additionally, should dep_v*.nim become mdep_v*.nim -- maybe not in this PR but eventually?

I tried looking up the convention, I do remember reading about m* prefix, but I can't remember the precise convention and I'm struggling to find it again. :(

Additionally, should dep_v*.nim become mdep_v*.nim

sure, noone will blame you if you do it in this PR :)

I can't remember the precise convention and I'm struggling to find it again. :(

https://nim-lang.github.io/Nim/contributing.html

Each test has its own file. All test files are prefixed with t. If you want to create a file for import into another test only, use the prefix m.

(slightly different context but same spirit)

no matter what system.nim will always get in there.

from system import nil as a 1st statement is an option.

from system import nil # or: from system import int, string ...

another option is filtering out "flaky things" that include line numbers from system.nim via execShellCmdEx based tests as shown above (see trunner.nim which relies on this pattern)

Things thus far:

deps_v*.nim are mdeps_v*.nim

this also stopped them from being compiled needlessly and executables being left behind

starting to create m*.nim files to isolate tests, tests are faster, more stable, and scenarios more precise

need to finish moving the rest over

m*.nim files might grow a fair bit, hopefully it's not a huge deal

from system import ... is a slick trick, for some reason it didn't occur to me that it would work with system 😅

Thanks again for your support -- it's really welcoming.

you're welcome: )

from system import ... is a slick trick, for some reason it didn't occur to me that it would work with system 😅

PR welcome to document it in https://nim-lang.github.io/Nim/system.html ; searching for from system in there returns nothing

Sadly, at least for nimsuggest this no longer works, I'm suspecting it was introduced as part of #15935.

Now what happens is that I get a whole lot of system.* suggestions immediatelly following the mstrutils.replace one. I haven't understood the IC stuff well enough to know what exactly is happening and more importantly whether this is in fact invalid.

As an aside I was poking as the VS Code extension to see if I could improve the integration with nimsuggest and this might also explain the increase in crashes. I'll have a further look tomorrow to see if I can suss out what's happening.

Output from the test failure -- I had tester.nim output more than 200 characters:

==== STDIN ====================================== Test failed: /home/saem/Development/nim/Nim/nimsuggest/tests/tdot4.nim Expected: *mstrutils.nim 10 5 "this is a test version of strutils.replace, it simply returns `by`" 100 None sug skFunc mstrutils.rereplace proc (s: string, sub: string, by: string): string{.noSideEffect, gcsafe, But got: /home/saem/Development/nim/Nim/nimsuggest/tests/mstrutils.nim 5 5 "this is a test version of strutils.replace, it simply returns `by`" 100 None sug skProc system.add proc (x: var string, y: string){.noSideEffect.} /home/saem/Development/nim/Nim/lib/system.nim 1026 5 "Concatenates `x` and `y` in place.\x0A\x0A.. code-block:: Nim\x0A var tmp = \"\"\x0A tmp.add(\"ab\")\x0A tmp.add(\"cd\")\x0A assert(tmp == \"abcd\")" 100 None sug skProc system.len proc (x: string): int{.noSideEffect.} /home/saem/Development/nim/Nim/lib/system.nim 697 5 "Returns the length of a string.\x0A\x0A.. code-block:: Nim\x0A var str = \"Hello world!\"\x0A echo len(str) # => 12" 100 None

Will address this in a future PR after I learn some more about what should happen.

nimsuggest/tests/tinclude.nim

nimsuggest/tests/tstrutils.nim

timotheecour · 2020-12-19T21:29:30Z

nimsuggest/tests/tsug_regression.nim

+# Open Questions:
+# * Why did hasKey sort above all?
+# * Why did getOrDefault drop off?
+# * Why does a deprecated procedures like `add` rank high?


Why did hasKey sort above all?
Why does a deprecated procedures like add rank high?

see implementation:

proc suggestEverything(c: PContext, n, f: PNode, outputs: var Suggestions) = # do not produce too many symbols: for (it, scopeN, isLocal) in allSyms(c): var pm: PrefixMatch if filterSym(it, f, pm): outputs.add(symToSuggest(c.config, it, isLocal = isLocal, ideSug, n.info, 0, pm, c.inTypeContext > 0, scopeN))

PR welcome to improve it using heuristics you suggest:
first, generate list of all acceptable candidates and add them to a priority queue, using same approach as what I'm doing in #16067; the priority in the queue can depend on scope (as done in that PR) plus heuristics (eg deprecated ranks lower, as you suggest)

then do walk the priority queue to get results in sorted order. Alternatively, using seq + sort instead of a priority queue is ok too, that's not the main point

Why did getOrDefault drop off?

no idea, I'm curious too; please instrument code (with echo, debug(n) from astalgo + friends) to figure out why getOrDefault is filtered out: does filterSym filter it out, or is it not even yieled inside suggestEverything?

Turns out suggestEverything is called much of the time, but not all the time. In this particular case the code ends up in sugExpr and since we have a nkDotExpr, it takes the suggestFieldAccess path and ends up in suggestObject and suggestOperations.

Any how, getOrDefault do in fact show up, just further down the list. While I was poking about, I found cmpSuggestions which compares Suggest based on various factors, one of which is quality (Range[0..100]). I think I should either:

have Suggest note whether something is deprecated and factor it into the comparator directly

have quality by way of the getQuality proc lower the quality rating

I'm not a fan of adding more state, but the former is most appealing as it means quality remains a sort of measure of relevance/closeness, while deprecation feels more like a tie breaker akin to boosting. NB. I'm borrowing terminology from fulltext indexing/search engines such as Elasticsearch when I say relevance and boosting.

Whatever the change, I'm inclined to make that in a follow-up PR, want to keep this focused on tests.

I don't care much how you implement it, but I completely agree that deprecated symbols should either not be suggested at all or be of low priority.

nimsuggest/tests/twithin_macro_prefix.nim

nimsuggest/tests/massertish.nim

alaviss · 2020-12-21T00:57:18Z

see if these can be made more unit test like skipping the full frontend

I would like to have the frontend properly tested as that's the part users will interact with. To speed things up we can just run tests in parallel (osproc.execProcesses can be used for that).

alaviss · 2020-12-21T00:59:50Z

Can we have test helper modules moved into a subfolder?

alaviss · 2020-12-21T01:09:21Z

nimsuggest/tests/twithin_macro.nim

+* from the errors that's either broken or not the way it works
+
+Open Questions:
+* how does nimsuggest transform the AST for suggestions or does it even?  


AFAIK, nimsuggest simply hooks into the compiler sem pass, so it does whatever the compiler would do when compiling the program. To get suggestions, nimsuggest have hook points in various places in the sem pass, then wait until sem process the AST node where the cursor is at, then access the symbol table of that scope to figure out what tokens are available.

I had a minor mental breakthrough when I saw the sem pass setup in nimsuggest. The question I wrote was without that nuance, but it's still relevant I think. Even though nimsuggest isn't doing it directly, templates, macros, etc... are all being evaluated, which means lots of computation & state change going on.

At the time I wrote the question and even now, my mind keeps coming back to:

semantic analysis of any nim code thrown at nimsuggest need to be forgiving

additionally any compile time execution needs to also be forgiving (templates, macros, ...)

state (expanded code, computed values, etc...) are going to have interesting impact on analysis and caching

What I'm trying to roughly point at is the above sounds like fertile ground for bugs unless and complexity in terms of usefully (correctly?) carrying out forgiving analysis and if I don't understand those bits better the chance of fixing a test like twithin_macro.nim seem rather slim. :/

Presently, the question could be refined to perhaps, "How does the semantic analysis that nimsuggest carry out via the sem pass transform the AST for generating suggestions, and how does evaluation of compile time code impact things, especially when carrying around or mutating state, carrying out an unbounded computation, employing IO, or allocating memory?". It might lose something in specificity.

IIRC, I've seen bugs like the following:

nimsuggest goes to 100% and sits there

unbounded computation or excessively hitting slow IO path because results weren't cached?

nimsuggest kept groing memory because of static

haven't read the fix yet so not sure what to make of it just yet

lack of idempotency, I've called the same call twice in a row and I get different results

reminds me of Haxe where certain macros must be authored with compilation cache awareness or :(

You know the answer to your questions already. There is some logic that prevents IO from being done in the VM for nimsuggest but all the other problems are there. However, ignoring the fact that plenty of code wasn't written with these restrictions in mind (hence bugs), the restrictions are not that hard to follow: There can be error types and nodes in the AST and processing should ignore them and continue.

@Araq thanks for confirming -- also provides some hints for potentially quick PRs.

I'm reading through notes and open questions and cleaning them up accordingly.

saem · 2020-12-21T07:23:58Z

see if these can be made more unit test like skipping the full frontend

I would like to have the frontend properly tested as that's the part users will interact with. To speed things up we can just run tests in parallel (osproc.execProcesses can be used for that).

I should have been more clear, I think there should be a few frontend tests to make sure the various bits of "wiring" are working for the frontend. Then hammer the rest with something that allows far more throughput. But that's wishful thinking presently.

Outside scope of PR.

saem · 2020-12-21T08:28:10Z

Can we have test helper modules moved into a subfolder?

~~Yeah, that sounds like a good clean-up before this PR is done -- once I've got the module names and structure settled a little bit more.~~

Done

saem · 2020-12-24T06:08:22Z

I'm inclined to squash all these into one commit -- just want to get confirmation from a reviewer before I do that. There isn't a particularly meaningful separation to be made as I played with a few different approaches.

timotheecour · 2020-12-26T11:19:24Z

I'm inclined to squash all these into one commit

that's fine (note that we now automatically enforce squashing before merging PR's anyways).
git rebase -i is the simplest

A number of nimsuggest tests were disabled for various reasons, sometimes due to brittleness. These tests have been fixed where needed and most have are now enabled -- details below. The updates are meant to provide better regression coverage for future nimsuggest improvements. To avoid brittleness some tests were refactored. Impact: * test coverage has now increased * faster execution of the test suite * tests are less likely to break due to stdlib changes Re-enabled Test & Test Description: * `tchk1.nim`: check (chk) via nimsuggest works at end of file * `tdot4.nim`: prioritize already used completion * `tinclude.nim`: definition lookup (def) with includes * `tstrutils.nim` -> `tdef2.nim`: test template definition lookup (def) * `tsug_regression.nim`: regression test for [nimsuggest nim-lang#52](nim-lang/nimsuggest#52) * `ttemplate_highlight.nim`: per the file name * `twithin_macro_prefix.nim`: suggest within a macro with a prefix Tests Not Re-Enabled: * `twithin_macro.nim` still disabled as it doesn't provide a good test signal * EPC highlight tests remain disabled -- requires out of scope tester changes Additional Notes: * todos added in comments for follow-up work

saem · 2020-12-26T20:29:37Z

I'm inclined to squash all these into one commit

that's fine (note that we now automatically enforce squashing before merging PR's anyways).
git rebase -i is the simplest

Done -- rebased on top of devel and squashed to a single commit.

A number of nimsuggest tests were disabled for various reasons, sometimes due to brittleness. These tests have been fixed where needed and most have are now enabled -- details below. The updates are meant to provide better regression coverage for future nimsuggest improvements. To avoid brittleness some tests were refactored. Impact: * test coverage has now increased * faster execution of the test suite * tests are less likely to break due to stdlib changes Re-enabled Test & Test Description: * `tchk1.nim`: check (chk) via nimsuggest works at end of file * `tdot4.nim`: prioritize already used completion * `tinclude.nim`: definition lookup (def) with includes * `tstrutils.nim` -> `tdef2.nim`: test template definition lookup (def) * `tsug_regression.nim`: regression test for [nimsuggest nim-lang#52](nim-lang/nimsuggest#52) * `ttemplate_highlight.nim`: per the file name * `twithin_macro_prefix.nim`: suggest within a macro with a prefix Tests Not Re-Enabled: * `twithin_macro.nim` still disabled as it doesn't provide a good test signal * EPC highlight tests remain disabled -- requires out of scope tester changes Additional Notes: * todos added in comments for follow-up work

timotheecour · 2021-02-10T10:00:41Z

@saem I'm having a hard time running tests locally:
nim c -r nimsuggest/tester fails with:

Test /Users/timothee/git_clone/nim/Nim_prs/nimsuggest/tests/taccent_highlight.nim
disabled epc: /Users/timothee/git_clone/nim/Nim_prs/nimsuggest/tests/taccent_highlight.nim
Test /Users/timothee/git_clone/nim/Nim_prs/nimsuggest/tests/tcallstrlit_highlight.nim
disabled epc: /Users/timothee/git_clone/nim/Nim_prs/nimsuggest/tests/tcallstrlit_highlight.nim
Test /Users/timothee/git_clone/nim/Nim_prs/nimsuggest/tests/tcase.nim
Test /Users/timothee/git_clone/nim/Nim_prs/nimsuggest/tests/tchk1.nim
==== STDIN ======================================

Test failed: /Users/timothee/git_clone/nim/Nim_prs/nimsuggest/tests/tchk1.nim
  Expected:  nestable statement requires indentation"   0
chk     skUnknown               Error   /Users/timothee/git_clone/nim/Nim_prs/nimsuggest/tests/tchk1.nim        12      0       "implementation of \'foo\' expected"    0
chk     skUnknown               Error   /Users/
  But got:   complex statement requires indentation"    0
chk     skUnknown               Error   /Users/timothee/git_clone/nim/Nim_prs/nimsuggest/tests/tchk1.nim        12      0       "implementation of \'foo\' expected"    0
chk     skUnknown               Error   /Users/t
==== EPC ========================================

...

  But got:
(return 567 nil)
/Users/timothee/git_clone/nim/Nim_prs/nimsuggest/tester.nim(347) tester
/Users/timothee/git_clone/nim/Nim_prs/nimsuggest/tester.nim(343) main
/Users/timothee/git_clone/nim/Nim_prs/nimsuggest/tester.nim(282) runEpcTest
/Users/timothee/git_clone/nim/Nim_prs/nimsuggest/tester.nim(179) sexpToAnswer
/Users/timothee/git_clone/nim/Nim_prs/lib/system/assertions.nim(30) failedAssertImpl
/Users/timothee/git_clone/nim/Nim_prs/lib/system/assertions.nim(23) raiseAssert
/Users/timothee/git_clone/nim/Nim_prs/lib/system/fatal.nim(53) sysFatal
Error: unhandled exception: /Users/timothee/git_clone/nim/Nim_prs/nimsuggest/tester.nim(179, 12) `m.kind == SList`  [AssertionDefect]

also note that
let libpath = findExe("nim").splitFile().dir /../ "lib"
should be:
let libpath = getCurrentCompilerExe().splitFile().dir /../ "lib"
otherwise you use a potentially different nim than the one used to run the test

A number of nimsuggest tests were disabled for various reasons, sometimes due to brittleness. These tests have been fixed where needed and most have are now enabled -- details below. The updates are meant to provide better regression coverage for future nimsuggest improvements. To avoid brittleness some tests were refactored. Impact: * test coverage has now increased * faster execution of the test suite * tests are less likely to break due to stdlib changes Re-enabled Test & Test Description: * `tchk1.nim`: check (chk) via nimsuggest works at end of file * `tdot4.nim`: prioritize already used completion * `tinclude.nim`: definition lookup (def) with includes * `tstrutils.nim` -> `tdef2.nim`: test template definition lookup (def) * `tsug_regression.nim`: regression test for [nimsuggest nim-lang#52](nim-lang/nimsuggest#52) * `ttemplate_highlight.nim`: per the file name * `twithin_macro_prefix.nim`: suggest within a macro with a prefix Tests Not Re-Enabled: * `twithin_macro.nim` still disabled as it doesn't provide a good test signal * EPC highlight tests remain disabled -- requires out of scope tester changes Additional Notes: * todos added in comments for follow-up work

saem changed the title ~~Saem nimsuggest renable tests~~ Saem nimsuggest re-enable tests Dec 19, 2020

timotheecour reviewed Dec 19, 2020

View reviewed changes

nimsuggest/tests/tinclude.nim Outdated Show resolved Hide resolved

timotheecour reviewed Dec 19, 2020

View reviewed changes

nimsuggest/tests/tstrutils.nim Outdated Show resolved Hide resolved

timotheecour reviewed Dec 19, 2020

View reviewed changes

nimsuggest/tests/twithin_macro_prefix.nim Outdated Show resolved Hide resolved

timotheecour reviewed Dec 19, 2020

View reviewed changes

nimsuggest/tests/twithin_macro_prefix.nim Outdated Show resolved Hide resolved

saem force-pushed the saem-nimsuggest-renable-tests branch from 6c4f384 to adb069d Compare December 20, 2020 03:54

timotheecour reviewed Dec 20, 2020

View reviewed changes

nimsuggest/tests/massertish.nim Outdated Show resolved Hide resolved

alaviss reviewed Dec 21, 2020

View reviewed changes

saem force-pushed the saem-nimsuggest-renable-tests branch from adb069d to 2fd2151 Compare December 21, 2020 08:26

saem force-pushed the saem-nimsuggest-renable-tests branch from ddcd86d to 90eac05 Compare December 23, 2020 08:47

saem marked this pull request as ready for review December 24, 2020 06:06

saem force-pushed the saem-nimsuggest-renable-tests branch from fc0ddde to 47e906b Compare December 26, 2020 20:28

Araq merged commit 4cf605d into nim-lang:devel Dec 27, 2020

timotheecour mentioned this pull request Feb 10, 2021

tables module uses runnableExamples #16994

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Saem nimsuggest re-enable tests #16401

Saem nimsuggest re-enable tests #16401

saem commented Dec 19, 2020 •

edited

Loading

saem commented Dec 19, 2020 •

edited

Loading

saem commented Dec 19, 2020

timotheecour Dec 19, 2020 •

edited

Loading

saem Dec 19, 2020

saem Dec 19, 2020

timotheecour Dec 19, 2020

saem Dec 20, 2020

timotheecour Dec 20, 2020

saem Dec 21, 2020

saem Dec 24, 2020

timotheecour Dec 19, 2020 •

edited

Loading

saem Dec 23, 2020

Araq Dec 27, 2020

alaviss commented Dec 21, 2020

alaviss commented Dec 21, 2020

alaviss Dec 21, 2020

saem Dec 23, 2020

Araq Dec 23, 2020

saem Dec 23, 2020

saem commented Dec 21, 2020 •

edited

Loading

saem commented Dec 21, 2020 •

edited

Loading

saem commented Dec 24, 2020

timotheecour commented Dec 26, 2020

saem commented Dec 26, 2020

timotheecour commented Feb 10, 2021

Saem nimsuggest re-enable tests #16401

Saem nimsuggest re-enable tests #16401

Conversation

saem commented Dec 19, 2020 • edited Loading

saem commented Dec 19, 2020 • edited Loading

saem commented Dec 19, 2020

timotheecour Dec 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour Dec 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alaviss commented Dec 21, 2020

alaviss commented Dec 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saem commented Dec 21, 2020 • edited Loading

saem commented Dec 21, 2020 • edited Loading

saem commented Dec 24, 2020

timotheecour commented Dec 26, 2020

saem commented Dec 26, 2020

timotheecour commented Feb 10, 2021

saem commented Dec 19, 2020 •

edited

Loading

saem commented Dec 19, 2020 •

edited

Loading

timotheecour Dec 19, 2020 •

edited

Loading

timotheecour Dec 19, 2020 •

edited

Loading

saem commented Dec 21, 2020 •

edited

Loading

saem commented Dec 21, 2020 •

edited

Loading