chore: adding NetConfig test suite #2091

gabrielmer · 2023-09-28T14:01:42Z

Description

Added test suite to cover common NetConfig settings

Changes

Created new file ./tests/test_waku_netconfig.nim with a NetConfig test suite
Moved defaultTestWakuNodeConf from test_app.nim to external_config.nim
Updated defaultTestWakuNodeConfto use our current default listenAddress 0.0.0.0 and added more default parameters

Issue

closes #1540

github-actions · 2023-09-28T14:01:56Z

This PR may contain changes to configuration options of one of the apps.

If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed.

Please also make sure the label release-notes is added to make sure any changes to the user interface are properly announced in changelog and release notes.

github-actions · 2023-09-28T14:07:27Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2091

Built from a4ab1a8

gabrielmer · 2023-09-28T14:07:40Z

tests/testlib/wakunode.nim

  ../../apps/wakunode2/internal_config,
-  ../wakunode2/test_app,


When importing defaultTestWakuNodeConf from test_app into other test suites, the test_app suite was being re-run as part of the suites importing it. For easier defaultTestWakuNodeConf's reusability, moved the function to external_config, mimicking newTestWakuNode, which is reused by many test suites and is defined in wakunode.nim

Ah, made a comment about this elsewhere before seeing this. wakunode.nim is a testlib - in other words, it's designed to be a utility module for functions useful in tests. newTestWakuNode defaultTestWakuNodeConf etc. shouldn't be defined in any "production" modules (test code should remain completely separate. The way to prevent test_app from being run multiple times for each import is to move defaultTestWakuNodeConf to a test helper module. I'd suggest just adding it to testlib/wakunode.nim and importing in both test_app and test_waku_netconfig

gabrielmer · 2023-09-28T14:10:45Z

tests/test_waku_netconfig.nim

+  check:
+    netConfig.announcedAddresses.len == 2  # External address + wsHostAddress
+    netConfig.announcedAddresses[1] == (ip4TcpEndPoint(extIp, 
+      conf.websocketPort) & wsFlag(wssEnabled))


Note that in order to configure WS using an extIp, the published port isn't extPort but conf.websocketPort. For websockets we bind to and announce the same port number.

jm-clius

Looks good and covers quite a lot! Thanks for this. Much better than what we have (which was noting 😅 ). Feel free to mark as ready for review once you think it can be merged.

jm-clius · 2023-09-28T16:59:00Z

tests/test_waku_netconfig.nim

+    check:
+      netConfigRes.isOk()
+
+asyncTest "AnnouncedAddresses contains only bind address when no external addresses are provided":


Indentation seem a bit weird.

jm-clius · 2023-09-28T17:00:27Z

tests/test_waku_netconfig.nim

+      netConfig.announcedAddresses[0] == formatListenAddress(ip4TcpEndPoint(conf.listenAddress, conf.tcpPort))
+
+
+asyncTest "AnnouncedAddresses contains external address if extIp/Port are provided":


Ditto in terms of indentation and for other tests (I think they should be single indented to be considered part of "Waku Netconfig" suite

Good catch! For some reason the tests were still being run as part of the test suite with that indentation, which is pretty interesting. Just pushed a fix for it - thank you!

jm-clius

LGTM. defaultTestWakuNodeConf should not be defined in a "production" module llike external_config.nim, but once this has been moved I'll be happy for the suite to be merged. No need to re-request review from my side. Great job!

jm-clius · 2023-09-29T10:20:17Z

apps/wakunode2/external_config.nim

+proc defaultTestWakuNodeConf*(): WakuNodeConf =
+  WakuNodeConf(
+    tcpPort: Port(60000),
+    websocketPort: Port(8000),
+    listenAddress: ValidIpAddress.init("0.0.0.0"),
+    rpcAddress: ValidIpAddress.init("127.0.0.1"),
+    restAddress: ValidIpAddress.init("127.0.0.1"),
+    metricsServerAddress: ValidIpAddress.init("127.0.0.1"),
+    dnsAddrsNameServers: @[ValidIpAddress.init("1.1.1.1"), ValidIpAddress.init("1.0.0.1")],
+    nat: "any",
+    maxConnections: 50,
+    topics: @["/waku/2/default-waku/proto"],
+    relay: true
+  )


Why has this been moved from test_app.nim? I wouldn't move any test code into "production" code.

jm-clius · 2023-09-29T10:26:59Z

tests/testlib/wakunode.nim

  ../../apps/wakunode2/internal_config,
-  ../wakunode2/test_app,


Ah, made a comment about this elsewhere before seeing this. wakunode.nim is a testlib - in other words, it's designed to be a utility module for functions useful in tests. newTestWakuNode defaultTestWakuNodeConf etc. shouldn't be defined in any "production" modules (test code should remain completely separate. The way to prevent test_app from being run multiple times for each import is to move defaultTestWakuNodeConf to a test helper module. I'd suggest just adding it to testlib/wakunode.nim and importing in both test_app and test_waku_netconfig

gabrielmer · 2023-09-29T10:56:32Z

LGTM. defaultTestWakuNodeConf should not be defined in a "production" module llike external_config.nim, but once this has been moved I'll be happy for the suite to be merged. No need to re-request review from my side. Great job!

Ooh now everything makes much more sense! I didn't notice that wakunode.nim was a testlib 😶
Will move defaultTestWakuNodeConf to wakunode.nim then :)

Thanks so much!

Ivansete-status

Thanks for that! You created many more test cases than I could imagine 🥇
Nevertheless, there are a few things that need to be revisited :)
It is especially important the comment about apps/wakunode2/external_config.nim. I'd prevent changing it.
Cheers!

tests/test_waku_netconfig.nim

waku/node/config.nim

SionoiS

No further comments than what was already said.

I just feel like we could use the type system in cases like this to enforce some "rules".

My 0.02$

gabrielmer · 2023-09-29T12:19:54Z

No further comments than what was already said.

I just feel like we could use the type system in cases like this to enforce some "rules".

My 0.02$

I'm not sure I fully understand the idea. Is it about setting explicitly the type when declaring variables instead of having the types being automatically inferred so we enforce the return types of procs?

Or what kind of rules are you thinking of?

Ivansete-status

LGTM! Thanks for it! 💯

SionoiS · 2023-09-29T12:55:42Z

No further comments than what was already said.
I just feel like we could use the type system in cases like this to enforce some "rules".
My 0.02$

I'm not sure I fully understand the idea. Is it about setting explicitly the type when declaring variables instead of having the types being automatically inferred so we enforce the return types of procs?

Or what kind of rules are you thinking of?

Probably not practical to do this here but the type system can help enforcing requirement like
AnnouncedAddresses contains only bind address when no external addresses are provided

For example you could use some type to represent the state.

only bind address when no external addresses are provided
only external address if extIp/Port are provided

Maybe the Nim type system is not made to do this though IDK. I only done this in Rust.

* chore: add retention policy with GB or MB limitation #1885 * chore: add retention policy with GB or MB limitation * chore: updated code post review- retention policy * ci: extract discordNotify to separate file Signed-off-by: Jakub Sokołowski <[email protected]> * ci: push images to new wakuorg/nwaku repo Signed-off-by: Jakub Sokołowski <[email protected]> * ci: enforce default Docker image tags strictly Signed-off-by: Jakub Sokołowski <[email protected]> * ci: push GIT_REF if it looks like a version Signed-off-by: Jakub Sokołowski <[email protected]> * fix: update wakuv2 fleet DNS discovery enrtree https://github.com/status-im/infra-misc/issues/171 * chore: resolving DNS IP and publishing it when no extIp is provided (#2030) * feat(coverage): Add simple coverage (#2067) * Add test aggregator to all directories. * Implement coverage script. * fix(ci): fix name of discord notify method Also use absolute path to load Groovy script. Signed-off-by: Jakub Sokołowski <[email protected]> * chore(networkmonitor): refactor setConnectedPeersMetrics, make it partially concurrent, add version (#2080) * chore(networkmonitor): refactor setConnectedPeersMetrics, make it partially concurrent, add version * add more metrics, refactor how most metrics are calculated * rework metrics table fillup * reset connErr to make sure we honour successful reconnection * chore(cbindings): Adding cpp example that integrates the 'libwaku' (#2079) * Adding cpp example that integrates the `libwaku` --------- Co-authored-by: NagyZoltanPeter <[email protected]> * fix(ci): update the dependency list in pre-release WF (#2088) * chore: adding NetConfig test suite (#2091) --------- Signed-off-by: Jakub Sokołowski <[email protected]> Co-authored-by: Jakub Sokołowski <[email protected]> Co-authored-by: Anton Iakimov <[email protected]> Co-authored-by: gabrielmer <[email protected]> Co-authored-by: Álex Cabeza Romero <[email protected]> Co-authored-by: Vaclav Pavlin <[email protected]> Co-authored-by: Ivan Folgueira Bande <[email protected]> Co-authored-by: NagyZoltanPeter <[email protected]>

chore: adding NetConfig test suite

fed75a7

gabrielmer commented Sep 28, 2023

View reviewed changes

Adding test cases for (ws) + dns + extIp

0b78fa4

jm-clius reviewed Sep 28, 2023

View reviewed changes

Fixing indentation

ccdaed8

gabrielmer marked this pull request as ready for review September 29, 2023 07:51

gabrielmer self-assigned this Sep 29, 2023

gabrielmer requested review from SionoiS, Ivansete-status and jm-clius September 29, 2023 07:53

jm-clius approved these changes Sep 29, 2023

View reviewed changes

Moving defaultTestWakuNodeConfg to testlib

a0e6f5b

Ivansete-status requested changes Sep 29, 2023

View reviewed changes

tests/test_waku_netconfig.nim Outdated Show resolved Hide resolved

tests/test_waku_netconfig.nim Show resolved Hide resolved

waku/node/config.nim Outdated Show resolved Hide resolved

SionoiS approved these changes Sep 29, 2023

View reviewed changes

gabrielmer added 2 commits September 29, 2023 14:56

Improving tests, including instead of importing private functions

e95b9ed

Deleting extra lines

5af720e

gabrielmer requested a review from Ivansete-status September 29, 2023 12:21

Ivansete-status approved these changes Sep 29, 2023

View reviewed changes

Merge branch 'master' into chore-improve-netconfig-test-coverage

9a25900

gabrielmer merged commit 23b49ca into master Sep 29, 2023
8 of 10 checks passed

gabrielmer deleted the chore-improve-netconfig-test-coverage branch September 29, 2023 13:30

ABresting pushed a commit that referenced this pull request Sep 30, 2023

chore: adding NetConfig test suite (#2091)

806c7b9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: adding NetConfig test suite #2091

chore: adding NetConfig test suite #2091

gabrielmer commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

github-actions bot commented Sep 28, 2023 •

edited

Loading

gabrielmer Sep 28, 2023

jm-clius Sep 29, 2023

gabrielmer Sep 28, 2023

jm-clius left a comment

jm-clius Sep 28, 2023

jm-clius Sep 28, 2023

gabrielmer Sep 29, 2023

jm-clius left a comment

jm-clius Sep 29, 2023

jm-clius Sep 29, 2023

gabrielmer commented Sep 29, 2023

Ivansete-status left a comment

SionoiS left a comment

gabrielmer commented Sep 29, 2023

Ivansete-status left a comment

SionoiS commented Sep 29, 2023 •

edited

Loading

		netConfig.announcedAddresses[0] == formatListenAddress(ip4TcpEndPoint(conf.listenAddress, conf.tcpPort))


		asyncTest "AnnouncedAddresses contains external address if extIp/Port are provided":

chore: adding NetConfig test suite #2091

chore: adding NetConfig test suite #2091

Conversation

gabrielmer commented Sep 28, 2023

Description

Changes

Issue

github-actions bot commented Sep 28, 2023

github-actions bot commented Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jm-clius left a 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

jm-clius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielmer commented Sep 29, 2023

Ivansete-status left a comment

Choose a reason for hiding this comment

SionoiS left a comment

Choose a reason for hiding this comment

gabrielmer commented Sep 29, 2023

Ivansete-status left a comment

Choose a reason for hiding this comment

SionoiS commented Sep 29, 2023 • edited Loading

github-actions bot commented Sep 28, 2023 •

edited

Loading

SionoiS commented Sep 29, 2023 •

edited

Loading