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

cmd/wavelet: fix unspecified wallet flag not generating random wallet #129

Conversation

hasyimibhar
Copy link
Contributor

@hasyimibhar hasyimibhar commented Jul 16, 2019

The documentation states that by not specifying the --wallet flag, a new wallet will be randomly generated. But currently, not specifying the --wallet flag will make Wavelet use config/wavelet.txt as the wallet. In order to to generate a random wallet, the --wallet flag has to be explicitly set to empty, e.g.:

$ ./wavelet --port 3000 --api.port 9000 --wallet ""

This PR fixes this bug, so that not specifying the --wallet flag will generate a random wallet. It also adds integration tests for cmd/wavelet. It requires wavelet binary to be in $PATH, so you need to install it if you want to run the test locally:

$ go install ./cmd/wavelet

The documentation states that by not specifying the --wallet flag, a new
wallet will be randomly generated[1]. But currently,  not specifying
the --wallet flag will make Wavelet use config/wavelet.txt as the
wallet. In order to to generate a random wallet, the --wallet flag has
to be explicitly set to empty.

[1] https://wavelet.perlin.net/docs/setup#wallet-management
@hasyimibhar
Copy link
Contributor Author

TestLimiterTimer seems to have intermittent failures. Even on my local machine it sometimes fail.

@a-urth
Copy link
Contributor

a-urth commented Jul 17, 2019

@hasyimibhar yes, kinda, it fails rarely though. Its non deterministic since it relies on timeout and on unknown factor which is speed of the system you running tests on.
i've noticed it couple of time too (literally 2 times so far), but anyway, thas my bad. i'll increase timeout to be 100% sure that it not conflicts with timeout.

@hasyimibhar hasyimibhar self-assigned this Jul 17, 2019
@iwasaki-kenta
Copy link
Contributor

Rather than assuming the Wavelet binary is in PATH, is it possible instead to just directly test the functions that make up the CLI?

I reckon that would be better, and make CI setup easier.

@hasyimibhar
Copy link
Contributor Author

Yeah it should be possible, and I think it's better as you can capture the test coverage as well. One way is to move all of the cli setup code outside of main and into its own function, so that what's left in main is something like this:

func main() {
    cmd := &Command{}
    os.Exit(cmd.Run(os.Args))
}

Then you can test cmd/wavelet like this:

func TestWavelet_Default(t *testing.T) {
        port := nextPort(t)
        apiPort := nextPort(t)

	cmd := &Command{}
	go runWavelet(t, cmd, []string{"--port", port, "--api.port", apiPort})
        defer cmd.Shutdown()

        // Wait for Wavelet to finish setup
        <-cmd.Wait()

        // Helper test method to call GET /ledger
	ledger := getLedgerStatus(t, apiPort)

	// Make sure a random wallet is generated
	assert.NotEqual(t, wallet1, ledger.PublicKey)
	assert.NotEqual(t, wallet2, ledger.PublicKey)
	assert.NotEqual(t, wallet3, ledger.PublicKey)
}

func runWavelet(t *testing.T, cmd *Command, args []string) {
    exitCode := cmd.Run(args)
    if exitCode != 0 {
        t.Fatal("non-zero exit code")
    }
}

But I noticed there's a major change in cmd/wavelet pending in #121, so I think it's better to wait for that PR to be merged first before proceeding.

@hasyimibhar
Copy link
Contributor Author

This PR is obsolete. Refer to #146 instead.

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.

3 participants