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

Adding utilities file #1

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Adding utilities file #1

wants to merge 18 commits into from

Conversation

AYAHASSAN287
Copy link
Collaborator

@AYAHASSAN287 AYAHASSAN287 commented Jan 12, 2025

This PR includes :

1- Adding initial structure of test framework as following

framework_design

1- tests include 1 test file example_test.go Testsuite "TestSubscribeNodetoTopic" does the following
- Config and create waku node
- Subscribe to default pubsubtopic
- get the ENR
- Stop and Destroy waku node
2- wrappers include wrapper file with APIs to wrap the waku-go internal APIs
3- utilities includes all support files to ease the functionality in wrappers & tests and logs file
4- libs contains waku-go internal APIs which is used directly from nwaku repo
5- nwaku repo is added as submodule in folder src

NOTE: framework will be modified un next task to allow dynamic assignation of parameters when creating new node

@fbarbu15 fbarbu15 self-requested a review January 13, 2025 14:12
Copy link
Collaborator

@fbarbu15 fbarbu15 left a comment

Choose a reason for hiding this comment

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

Structure looks good to me
I like how the wrappers are used
What I'm not sure if it exists and we will surely need is logging capabilities. Ideally both test logs (what the test framework is dooing) and also node logs (what waku is logging)

Copy link
Collaborator

Choose a reason for hiding this comment

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

it should be nwaku internal APIs

@AYAHASSAN287
Copy link
Collaborator Author

Structure looks good to me I like how the wrappers are used What I'm not sure if it exists and we will surely need is logging capabilities. Ideally both test logs (what the test framework is dooing) and also node logs (what waku is logging)

Yes I agree . I will add logging module and use it in wrappers & tests

@AYAHASSAN287 AYAHASSAN287 requested a review from fbarbu15 January 17, 2025 10:24
@fbarbu15 fbarbu15 requested a review from romanzac January 17, 2025 11:57
Copy link
Collaborator

@fbarbu15 fbarbu15 left a comment

Choose a reason for hiding this comment

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

Thanks Aya, code looks good in most of the parts.
Left some minor comments.
Also @romanzac please review it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think waku internal APIs is better instead of go-waku

Copy link
Collaborator

Choose a reason for hiding this comment

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

What does each arrow mean? not sure I can follow the diagram.
But I agree that go-waku should probably be out of the picture

Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need this file to be commited?

return nil
}

func StartWakuNodeWithDefaultValues(host string, port int, nodeKey string, enableRelay bool, logLevel string) (*LocalWakuNode, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the function parameters are not used
If it's with default values then probably you don't need no params

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we have some linters that could catch such issues automatically?
@romanzac do you know of such tools in golang?

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we have some linters that could catch such issues automatically? @romanzac do you know of such tools in golang?

Yes, I have used golangci-lint

return nil, err
}

fmt.Println("WakuNode created successfully!")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we should use the logger and not print

EnableRelay: enableRelay,
LogLevel: logLevel,
LocalConfigData: golang.WakuConfig{
Host: utilities.IfEmpty(host, "127.0.0.1"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should define the defaults/constants into a separate module/file

func (node *WakuNode) RelayUnsubscribe(pubSubTopic string) error {
return node.WakuRelayUnsubscribe(pubSubTopic)
// Function publishes a message to a pubsub topic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

same

"time"
"waku-go-bindings-tests/src/libs"
"waku-go-bindings-tests/src/utilities"
//"github.com/stretchr/testify/assert"
Copy link
Collaborator

Choose a reason for hiding this comment

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

why commented, shouldn't the test contain some assertings?

}
}()

utilities.LogDebug("Sleep for 2 seconds")
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we should have a utility function called delay that can also do the sleep and log the reasons for the sleep ex
utilities.delay(2 * time.Second, "Sleep for 2 seconds because...")

Copy link
Collaborator

Choose a reason for hiding this comment

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

this file should eb gitignored

NodeKey: "11d0dcea28e86f81937a3bd1163473c7fbc0a0db54fd72914849bc47bdf78710",
EnableRelay: true,
LogLevel: "DEBUG",
func TestSubscribeNodetoTopic(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice test but would be even nicer if we could have 2 nodes connected between them, one node sends a message and the 2nd one receives it

@fbarbu15
Copy link
Collaborator

Also a question, should we use nwaku to build the waku go bindings or https://github.com/waku-org/waku-go-bindings?
cc @gabrielmer

@fbarbu15 fbarbu15 requested a review from gabrielmer January 17, 2025 12:10
Copy link
Collaborator

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Thanks so much! 😍

We should integrate https://github.com/waku-org/waku-go-bindings instead of the golang examples in the nwaku repo. As you will see, that repo is much more developed and has its features tested at a high level. I think it will make the work of testing way easier, there's already lots of functionalities developed and examples/tests to use as reference.

Great job so far!

Copy link
Collaborator

Choose a reason for hiding this comment

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

What does each arrow mean? not sure I can follow the diagram.
But I agree that go-waku should probably be out of the picture


3. **Modify Package Name:**
- Navigate to `src/nwaku/example/golang`
Copy link
Collaborator

@gabrielmer gabrielmer Jan 17, 2025

Choose a reason for hiding this comment

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

We should test the interfaces provided by the https://github.com/waku-org/waku-go-bindings/tree/master repo instead of the golang examples in nwaku

I also created a super simple example of waku-go-bindings integrated into a Go project here

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think many of these wrappers won't be necessary once we use waku-go-bindings instead of the example? Maybe I'm missing something, but the wrappers look very similar to existing functions

Copy link
Collaborator

Choose a reason for hiding this comment

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

Notice that in https://github.com/waku-org/waku-go-bindings/blob/master/waku/nwaku_test.go there's simple tests for most of the features that can be used as reference

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @gabrielmer I don't think we were aware of this repo, at least I wasn't until today.
Yes, it seems that this already has a lot of stuff implemented, nice!
Should we create tests directly in this repo?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apologies for the misunderstanding!

Either in that repo or in a separate one are good for me :)) I think it makes sense to have everything in one repo, but if there's any upside of doing it separately then it's also good.

@Ivansete-status @richard-ramos any preferences?

@gabrielmer
Copy link
Collaborator

Also a question, should we use nwaku to build the waku go bindings or https://github.com/waku-org/waku-go-bindings? cc @gabrielmer

We should use https://github.com/waku-org/waku-go-bindings :)

Let me know if you run into any issues using it! The README explains how to integrate it into a new Go project, and we have a small example of a project in which it's integrated

@gabrielmer
Copy link
Collaborator

Oh an before I forget! Not sure if Store enters to the scope of the testing, but it currently doesn't work in waku-go-bindings. However, I have a PR open adding some missing things and including a test for Store

So if later we need to add more Store tests, we will be able to use it as a reference. That PR can take some days to be merged because there's a breaking change in nwaku we are introducing and we might need to wait a few days to update their dependent projects. So don't try to use Store until that is merged :))

return node.WakuVersion()
// Function to return formatted contetn topic
func (localNode *LocalWakuNode) FormatContentTopic(appName string, appVersion int,
contentTopicName string,
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it just me or the formating here is off? @romanzac what do you use for code formatting in go?

I'm using JetBrains IDE. Command line is https://go.dev/blog/gofmt.

return defaultValue
}
return value
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great work so far!

Depends how much you would like to learn Go. I would recommend to have a look at generics. It could sometimes save typing:

package utilities

func OrDefault[T comparable](val, def T) T {
	var zero T
	if val == zero {
		return def
	}
	return val
}

Usage:

s := OrDefault("", "default string")

i := OrDefault(0, 42)

Interesting book I own myself as well: https://100go.co/

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.

4 participants