fix(tests): add missing import for tests #1375

alrevuelta · 2022-11-14T10:18:50Z

Due to a missing import, some tests could not be run isolated. The issue can be reproduced as follows, and it applies to all modified tests where the chronicles import was added.

$ ./env.sh bash
$ nim c -r ./tests/v2/test_waku_filter.nim

/Users/xxx/Github/nwaku/tests/v2/test_waku_filter.nim(18, 95) template/generic instantiation of `async` from here
/Users/xxx/Github/nwaku/tests/v2/test_waku_filter.nim(20, 30) template/generic instantiation of `new` from here
/Users/xxx/Github/nwaku/waku/v2/node/peer_manager/peer_manager.nim(149, 11) template/generic instantiation of `debug` from here
/Users/xxx/Github/nwaku/vendor/nim-chronicles/chronicles.nim(359, 10) template/generic instantiation of `log` from here
/Users/xxx/Github/nwaku/vendor/nim-chronicles/chronicles.nim(332, 21) Error: undeclared identifier: 'activeChroniclesStream'
candidates (edit distance, scope distance); see '--spellSuggest':
 (13, 7): 'IfDvbrccUpstream' [enumField declared in /Users/xxx/Github/nwaku/vendor/nim-chronos/chronos/transports/osnet.nim(176, 5)]
 (13, 7): 'asyncstream' [module declared in /Users/xxx/Github/nwaku/vendor/nim-chronos/chronos/streams/chunkstream.nim(12, 8)]
 (13, 7): 'asyncstream' [module declared in /Users/xxx/Github/nwaku/vendor/nim-chronos/chronos/transport.nim(10, 17)]
 (13, 7): 'chunkstream' [module declared in /Users/xxx/Github/nwaku/vendor/nim-chronos/chronos/transport.nim(10, 17)]
 (13, 7): 'initStream' [method declared in /Users/xxx/Github/nwaku/vendor/nim-libp2p/libp2p/protocols/secure/secure.nim(63, 8)]
 (13, 7): 'initStream' [method declared in /Users/xxx/Github/nwaku/vendor/nim-libp2p/libp2p/protocols/secure/secure.nim(63, 8)]
 (13, 7): 'initStream' [method declared in /Users/xxx/Github/nwaku/vendor/nim-libp2p/libp2p/stream/connection.nim(75, 8)]
 (13, 7): 'initStream' [method declared in /Users/xxx/Github/nwaku/vendor/nim-libp2p/libp2p/stream/connection.nim(75, 8)]
 (13, 7): 'initStream' [method declared in /Users/xxx/Github/nwaku/vendor/nim-libp2p/libp2p/stream/lpstream.nim(154, 8)]
 (13, 7): 'initStream' [method declared in /Users/xxx/Github/nwaku/vendor/nim-libp2p/libp2p/stream/lpstream.nim(154, 8)]

rymnc

LGTM, thanks!

rymnc · 2022-11-14T10:22:16Z

waku/v2/README.md

 ```bash
 # Get a shell with the right environment variables set
 ./env.sh bash
 # Run a specific test
 nim c -r ./tests/v2/test_waku_filter.nim
 ```

+And and alter compile options as you want. For example, if you want a less verbose output you can do the following. For more, refer to the [compiler flags](https://nim-lang.org/docs/nimc.html#compiler-usage) and [chronicles documentation](https://github.com/status-im/nim-chronicles#compile-time-configuration).


Suggested change

And and alter compile options as you want. For example, if you want a less verbose output you can do the following. For more, refer to the [compiler flags](https://nim-lang.org/docs/nimc.html#compiler-usage) and [chronicles documentation](https://github.com/status-im/nim-chronicles#compile-time-configuration).

and alter compile options as you want. For example, if you want a less verbose output you can do the following. For more, refer to the [compiler flags](https://nim-lang.org/docs/nimc.html#compiler-usage) and [chronicles documentation](https://github.com/status-im/nim-chronicles#compile-time-configuration).

status-im-auto · 2022-11-14T10:34:40Z

Jenkins Builds

Click to see older builds (8)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`865e7b3`	#1	2022-11-14 10:34:40	~15 min	`macos`	📦`bin`
❌	`865e7b3`	#1	2022-11-14 10:35:02	~15 min	`linux`	📄`log`

✔️	`10dd360`	#2	2022-11-14 14:13:31	~15 min	`linux`	📦`bin`
✔️	`10dd360`	#2	2022-11-14 14:22:25	~24 min	`macos`	📦`bin`

✔️	`873e587`	#3	2022-11-17 15:50:31	~15 min	`linux`	📦`bin`
✔️	`873e587`	#3	2022-11-17 15:55:31	~20 min	`macos`	📦`bin`

❌	`e4de90c`	#4	2022-11-18 10:52:10	~15 min	`linux`	📄`log`
✔️	`e4de90c`	#4	2022-11-18 10:54:06	~17 min	`macos`	📦`bin`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`1465de7`	#5	2022-11-21 11:27:17	~15 min	`linux`	📦`bin`
✔️	`1465de7`	#5	2022-11-21 11:31:48	~19 min	`macos`	📦`bin`

✔️	`d8ccfc7`	#6	2022-11-21 14:36:21	~14 min	`linux`	📦`bin`
✔️	`d8ccfc7`	#6	2022-11-21 14:43:40	~22 min	`macos`	📦`bin`

LNSD

Changes LGTM ✅

One question: are this changes adding an UnusedImport warning to the test2 compilation logs? If so, then I would rather investigate a better approach that do not introduce those warnings and allows compiling the module individually.

Some context:

The amount of warnings we have is so high that is already causing "alert fatigue", and we can miss relevant issues for the PR. We need to address them as a maintenance chore.

LNSD · 2022-11-14T12:13:37Z

are these changes adding an UnusedImport warning to the test2 compilation logs?

I answer my question: Yes, they do.

https://github.com/status-im/nwaku/actions/runs/3460521398/jobs/5777145106#step:7:44

Then, I would investigate a better approach that does not introduce those warnings and allows compiling the module individually.

jm-clius

LGTM! Thanks for fixing this, as these individual tests have been broken for a while.

I agree with @LNSD about perhaps finding a way of doing this that does not increase compiler warnings (we've been struggling to stay ahead here due to Nim limitations). Is it possible to use the {.used.} pragma (explained here) on the specific offending imports?
If there's no easy and clean way of avoiding the warnings, I think we should track it in a separate issue and come up with a strategy, but that would be subsequent to fixing the tests.

alrevuelta · 2022-11-14T14:06:05Z

@LNSD good catch. A simple approach could be to use this but I wouldn't like to have the test code full of these off/on statements. Another approach is to find the root cause of the issue and fix it properly, but have the feeling this will take some time. As I see it this has a really low prio, so I would manually disable the warnings as @jm-clius kind of suggested.

LNSD · 2022-11-14T14:24:30Z

so I would manually disable the warnings as @jm-clius kind of suggested.

What do you mean by "manually disable" the warnings? 👀

alrevuelta · 2022-11-14T14:43:36Z

What do you mean by "manually disable" the warnings? 👀

Disregard my comment. I was expecting this to work, but it does not.

{. warning[UnusedImport]:off .}

import "sequtils"

{. warning[UnusedImport]:on .}

import "tables"

echo "hi there"

But:

Warning: imported and not used: 'sequtils' [UnusedImport]
Warning: imported and not used: 'tables' [UnusedImport]

I was ofc expecting to warn only about tables.

LNSD · 2022-11-16T12:58:33Z

What are the plans for this PR? Shall we close it and open an issue to investigate a solution?

cc @jm-clius @alrevuelta

alrevuelta · 2022-11-16T13:24:48Z

@LNSD will give it a shot before the end of the week. if it turns out to be too much, i would merge the PR and open an issue to track it. I think its better a "unused import" than a compile error. wdyt?

jm-clius · 2022-11-17T12:48:47Z

Let's merge this. The issue here is incorrect compiler message of unused import for an import that's clearly used. First step is to fix the tests, which indeed affects development (I ran into this as well). Next step is to see if there is a solution around this limitation of the Nim compiler.

alrevuelta · 2022-11-17T15:43:53Z

@jm-clius great! tracking it here: #1398

alrevuelta requested review from LNSD, rymnc and jm-clius November 14, 2022 10:19

alrevuelta marked this pull request as ready for review November 14, 2022 10:19

rymnc approved these changes Nov 14, 2022

View reviewed changes

rymnc reviewed Nov 14, 2022

View reviewed changes

LNSD reviewed Nov 14, 2022

View reviewed changes

jm-clius approved these changes Nov 14, 2022

View reviewed changes

alrevuelta force-pushed the fix-tests branch from 865e7b3 to 10dd360 Compare November 14, 2022 13:57

alrevuelta force-pushed the fix-tests branch from 10dd360 to 873e587 Compare November 17, 2022 15:35

alrevuelta mentioned this pull request Nov 17, 2022

chore: unused import warning in tests #1398

Open

alrevuelta force-pushed the fix-tests branch 2 times, most recently from e4de90c to 1465de7 Compare November 21, 2022 11:11

fix(tests): add missing import for tests

d8ccfc7

alrevuelta force-pushed the fix-tests branch from 1465de7 to d8ccfc7 Compare November 21, 2022 14:21

alrevuelta merged commit 1e8e60c into master Nov 22, 2022

alrevuelta deleted the fix-tests branch November 22, 2022 07:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(tests): add missing import for tests #1375

fix(tests): add missing import for tests #1375

alrevuelta commented Nov 14, 2022

rymnc left a comment

rymnc Nov 14, 2022

status-im-auto commented Nov 14, 2022 •

edited

Loading

LNSD left a comment

LNSD commented Nov 14, 2022

jm-clius left a comment

alrevuelta commented Nov 14, 2022

LNSD commented Nov 14, 2022

alrevuelta commented Nov 14, 2022

LNSD commented Nov 16, 2022

alrevuelta commented Nov 16, 2022 •

edited

Loading

jm-clius commented Nov 17, 2022

alrevuelta commented Nov 17, 2022

	And and alter compile options as you want. For example, if you want a less verbose output you can do the following. For more, refer to the [compiler flags](https://nim-lang.org/docs/nimc.html#compiler-usage) and [chronicles documentation](https://github.com/status-im/nim-chronicles#compile-time-configuration).
	and alter compile options as you want. For example, if you want a less verbose output you can do the following. For more, refer to the [compiler flags](https://nim-lang.org/docs/nimc.html#compiler-usage) and [chronicles documentation](https://github.com/status-im/nim-chronicles#compile-time-configuration).

fix(tests): add missing import for tests #1375

fix(tests): add missing import for tests #1375

Conversation

alrevuelta commented Nov 14, 2022

rymnc left a comment

Choose a reason for hiding this comment

rymnc Nov 14, 2022

Choose a reason for hiding this comment

status-im-auto commented Nov 14, 2022 • edited Loading

Jenkins Builds

LNSD left a comment

Choose a reason for hiding this comment

LNSD commented Nov 14, 2022

jm-clius left a comment

Choose a reason for hiding this comment

alrevuelta commented Nov 14, 2022

LNSD commented Nov 14, 2022

alrevuelta commented Nov 14, 2022

LNSD commented Nov 16, 2022

alrevuelta commented Nov 16, 2022 • edited Loading

jm-clius commented Nov 17, 2022

alrevuelta commented Nov 17, 2022

status-im-auto commented Nov 14, 2022 •

edited

Loading

alrevuelta commented Nov 16, 2022 •

edited

Loading