multi: fix dcrwallet/errors usage #92

beansgum · 2020-01-18T04:20:51Z

Errors were wrongly formatted and it returned an unreadable error. The fixes it by using the correct error formatting function.
Closes #90 & #91

log.go

oluwandabira · 2020-01-18T04:24:24Z

log.go

 }

 // RegisterLogger should be called before logRotator is initialized.
 func RegisterLogger(tag string) (slog.Logger, error) {
 	if logRotator != nil {
-		return nil, fmt.Errorf("cannot register logger after log rotator is initialized")
+		return nil, errors.E("cannot register logger after log rotator is initialized")


Again, why not use fmt?

github.com/decred/dcrwallet/errors is used all through the project

But can't we use concrete error types? Searching the error strings for the exact error isn't that nice.

multiwallet.go

syncnotification.go

oluwandabira

You're still using strings, not error types

log.go

utils/netparams.go

multiwallet.go

multiwallet_test.go

itswisdomagain · 2020-01-18T23:53:07Z

multiwallet_test.go

+// If will delete the directory if it is empty.
+func canUseDir(directory string) bool {
+	if os.MkdirAll(directory, os.ModePerm) == nil {
+		return os.Remove(directory) == nil


Considering that this function is used to test the NewMultiwallet function, this should return nil. NewMultiWallet only attempts to create the directory if it does not exist, it does not remove any existing content from the directory - which is important since the root dir may contain a wallet.db file that will/should be linked to the new multiwallet instance, AFTER, the instance is created.

Yeah, I sure knew that since it was documented somewhere...

And the thing is that this removes the directory if it exists but is empty (like the code comment said) so all values for which canUseDir returns true are valid paths that do not exist.

Ah I see. I guess that's the point I'm trying to make. A path may also be valid if it exists and is not empty. This function would return an error for any valid path that is not empty.

It doesn't return an error...

multiwallet.go

itswisdomagain · 2020-01-19T06:53:33Z

multiwallet_test.go

+// If will delete the directory if it is empty.
+func canUseDir(directory string) bool {
+	if os.MkdirAll(directory, os.ModePerm) == nil {
+		return os.Remove(directory) != nil


Still won't work. os.Remove will be nil if the directory is empty. Doesn't make the directory unusable. My recommendation:

Suggested change

return os.Remove(directory) != nil

return true

It passes...

Why wouldn't an empty dir be usable?

It is usable. But following your code, an empty directory will return false here because os.Remove will be nil.

It can't be empty if it got here, os.MkdirAll creates it?

os.MkdirAll creates an empty directory.

Oh yeah, was confused for a sec

itswisdomagain

To summarise, there are 2 expected adjustments:

The canUseDir function should only check if the directory can be created if did not exist. It should not check if it can be deleted, because it can be deleted if it is empty and cannot be deleted if non-empty, in either case, the directory is still valid - whether empty or not.
The test function itself should not expect the error from NewMultiwallet to be nil just because the directory is valid. It should, however, expect the error to be non-nil, if the directory is invalid. As a matter of fact, it should expect the error to be of type os.PathError.

Errors were wrongly formatted and it returned an unreadable error. The fixes it by using the correct error formatting function.

itswisdomagain · 2020-01-22T09:11:25Z

.github/workflows/build.yml

        run: |
-          export PATH=$PATH:~/go/bin
-          ./run_tests.sh lint


With this change, we'd need to remove the ./run_tests.sh file as well. However, I'd recommend keeping it. Should probably rename it though, to say ./lint.sh.

itswisdomagain · 2020-01-22T09:11:47Z

.github/workflows/build.yml

-      - name: Install linter
-        run: "curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $(go env GOPATH)/bin v1.22.2"


If we're keeping the linter sh file, we'll need to retain this as well.

itswisdomagain · 2020-01-22T09:15:24Z

.github/workflows/build.yml

+        env:
+          GO111MODULE: "on"


While we're here, I'd recommend moving this to before run.

itswisdomagain · 2020-01-22T09:16:04Z

multiwallet_test.go

+)
+
+// canUseDir returns true if the program can create the directory
+// It will create the directory if it can


This comment is unnecessary. The previous line covers it.

itswisdomagain · 2020-01-22T09:16:12Z

multiwallet_test.go

+	. "github.com/raedahgroup/dcrlibwallet"
+)
+
+// canUseDir returns true if the program can create the directory


Suggested change

// canUseDir returns true if the program can create the directory

// canUseDir returns true if the program can create the directory.

itswisdomagain · 2020-01-22T09:17:09Z

multiwallet_test.go

+}
+
+// TestNewMultiWalletPath checks that NewMultiWallet returns an error for invalid
+// paths or no error for valid paths


Suggested change

// paths or no error for valid paths

// paths

Valid paths may still return an error.

itswisdomagain · 2020-01-22T09:24:49Z

multiwallet_test.go

+		canUse := canUseDir(rootDir)
+		_, err := NewMultiWallet(rootDir, "", "testnet")
+		if canUse {
+			return err == nil


As has been explained previously, this test is invalid because canUse can be true, yet err != nil. Using a valid path DOES NOT always mean that NewMultiWallet will return no errors; it might, it might not.

It all boils down to what is being tested in this function. I was even gonna recommend renaming the test function as this testing code does not perform a complete test of the NewMultiWallet function. As indicated in the comment, it all tests path validity.

If during a test, the path is valid AND an error is returned, this test will fail when it should not. Reason: if a test fails, it means there's a problem somewhere - either with the code being tested or the testing code itself. In the scenario highlighted above, the test would fail, not necessarily because there is a problem with the NewMultiWallet function, but because there's a problem with this testing code.

itswisdomagain · 2020-01-22T11:34:13Z

multiwallet_test.go

+	f := func(rootDir string) bool {
+		canUse := canUseDir(rootDir)
+		_, err := NewMultiWallet(rootDir, "", "testnet")
+		if canUse {
+			return err == nil
+		}
+		return err != nil
+	}


As have been said previously, this test is buggy. I made some changes to the f function, ran the test a couple times to get 2 set of results (one passed, one failed) which reveal some issues with this testing code. The log outputs displayed for both results are truncated.

Modified the f function to print somewhat detailed reports

f := func(rootDir string) bool { canUse := canUseDir(rootDir) _, err := NewMultiWallet(rootDir, "", "testnet") if canUse { passed := err == nil var report string if passed { report = fmt.Sprintf("GOOD: %U is a valid path and we got no errors. Yay!", rootDir) } else { report = fmt.Sprintf("BAD: %U is a valid path but we got an error: %s.", rootDir, err.Error()) } println(report) return passed } passed := err != nil var report string if passed { report = fmt.Sprintf("GOOD: %U is NOT a valid path, so we got an error: %s.", rootDir, err.Error()) } else { report = fmt.Sprintf("BAD: %U is NOT a valid path but we got no errors. Weird!", rootDir) } println(report) return passed }

First test result + log output (TEST PASSED with exit code 0, but should it have?)

GOOD: "\U000cbb31\U0006ecc1⋯\U000859ac𮗚\U0008311f\U000df8a3\U00076647" is NOT a valid path, so we got an error: invalid net type. GOOD: "\U0008ddd5𡢙\U000c97e4\U00106c29\U000ac4b7̌\U000f4f23\U0004faa2\U000547ee\U0003e399\U00046378\U0001f269\U0010a688𬡚\U0009449d𗢷" is NOT a valid path, so we got an error: invalid net type. GOOD: "\U0001ecac\U00095bba奕\U000782e0\U000ceb5b\U000b03f1\U0007d731\U00092766𨞈\U000a9ae0\U000aebf2" is NOT a valid path, so we got an error: invalid net type. GOOD: "\U00066d7c\U000c02a8" is NOT a valid path, so we got an error: invalid net type. GOOD: "\U0005214c\U00075f48\U0004562f\U0004ee05" is NOT a valid path, so we got an error: invalid net type. GOOD: "\U00054699\U00097366\U0004709a" is NOT a valid path, so we got an error: invalid net type. GOOD: "\U00085c61\U00042df4\U000149ee\U0006f80c\U000ada6e\U00067402" is NOT a valid path, so we got an error: invalid net type. GOOD: "\U00050e5a\U00043f9f贉\U0007b60f\U0005f314\U000bd61d\U0005847b�\U000622ce\U000671f0췢\U0010a726\U000e4da9" is NOT a valid path, so we got an error: invalid net type. Process finished with exit code 0

Analysis

All test parameters passed to f were invalid paths. So it is expected that NewMultiWallet will return an error; and it did! But the error returned was invalid net type instead of a PathError. Agreed, if the netType was indeed valid, the error would have been a PathError, but this just goes to emphasize that the NewMultiWallet function can return an error whether or not a valid path is provided.

If a valid netType was passed, the error would look like GOOD: "\U0007af92\U0010ac8d\U0009d6ae" is NOT a valid path, so we got an error: failed to create rootDir: mkdir 񺾒􊲍򝚮: illegal byte sequence. This kind of error should be what is used in determining whether the test should pass or fail. Any error shouldn't do.

Second test result + log output (TEST FAILED with exit code 1, but should it have?)

GOOD: "" is NOT a valid path, so we got an error: invalid net type. GOOD: "\U000cd531\U00030f92\U0006d2cf\U000f2b31" is NOT a valid path, so we got an error: invalid net type. GOOD: "\U001018f1\U000c57d4𨃁\U00101e3f\U00107546" is NOT a valid path, so we got an error: invalid net type. GOOD: "\U000d9d16" is NOT a valid path, so we got an error: invalid net type. GOOD: "" is NOT a valid path, so we got an error: invalid net type. GOOD: "" is NOT a valid path, so we got an error: invalid net type. GOOD: "𗰶𨺈䵄\U000eca4a\U000c2b42" is NOT a valid path, so we got an error: invalid net type. GOOD: "\U0006d593𤣯\U000f1629" is NOT a valid path, so we got an error: invalid net type. BAD: "\ue91b" is a valid path but we got an error: invalid net type. multiwallet_test.go:52: #47: failed on input "\ue91b" Process finished with exit code 1

Analysis

The test failed even though the path was valid. This shouldn't be.

An error may be returned even when the path is valid. Therefore, it is incorrect to expect no errors simply because the path is valid.

This the reason the last test run on this PR failed. It shouldn't have.

Other observations:

It turns out that relative paths are supported but whether or not this is desired is yet to be seen.

canUseDir creates the directory if it does not exist, should also ensure that the directory is deleted IF it was created. Should not delete the directory if it was already existing.

An interesting edge case

If the test code is corrected to use a valid netType (say testnet3), the test may still fail with the following error:

BAD: "" is NOT a valid path but we got no errors. Weird! 2020-01-22 12:20:57.101 [INF] DLWL: Loaded 0 wallets multiwallet_test.go:52: #90: failed on input "" Process finished with exit code 1

This means that the NewMultiWallet function had no issues using the directory (no errors returned from the function) but canUseDir said the directory is not valid. Who's wrong?

oluwandabira suggested changes Jan 18, 2020

View reviewed changes

oluwandabira self-requested a review January 18, 2020 04:35

oluwandabira reviewed Jan 18, 2020

View reviewed changes

itswisdomagain suggested changes Jan 18, 2020

View reviewed changes

itswisdomagain suggested changes Jan 19, 2020

View reviewed changes

beansgum and others added 6 commits January 20, 2020 01:53

multi: fix dcrwallet/errors usage

b954a8e

Errors were wrongly formatted and it returned an unreadable error. The fixes it by using the correct error formatting function.

return error for invalid net type

dc9d846

use concrete error type

974187b

Add tests for NewMultiWallet

31ff194

replace 0700 -> os.ModePerm

c1f5c49

Change canUseDir condition

2a4a1d9

oluwandabira force-pushed the error_format branch from ef9a70f to 2a4a1d9 Compare January 19, 2020 23:54

oluwandabira added 2 commits January 20, 2020 01:57

Change Build and Test to work on pull requests

56f7de9

Correct NewMultiTest

f2c4060

itswisdomagain suggested changes Jan 22, 2020

View reviewed changes

Revert test changes

316126f

oluwandabira approved these changes Jan 22, 2020

View reviewed changes

itswisdomagain approved these changes Jan 22, 2020

View reviewed changes

itswisdomagain merged commit 064d4c3 into planetdecred:master Jan 22, 2020

beansgum deleted the error_format branch January 22, 2020 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: fix dcrwallet/errors usage #92

multi: fix dcrwallet/errors usage #92

beansgum commented Jan 18, 2020

oluwandabira Jan 18, 2020

beansgum Jan 18, 2020

oluwandabira Jan 18, 2020

oluwandabira Jan 18, 2020

oluwandabira left a comment

itswisdomagain Jan 18, 2020

oluwandabira Jan 19, 2020

oluwandabira Jan 19, 2020

itswisdomagain Jan 19, 2020

oluwandabira Jan 19, 2020

itswisdomagain Jan 19, 2020

oluwandabira Jan 19, 2020

oluwandabira Jan 19, 2020

itswisdomagain Jan 19, 2020

oluwandabira Jan 19, 2020

itswisdomagain Jan 19, 2020

oluwandabira Jan 19, 2020

itswisdomagain left a comment

itswisdomagain Jan 22, 2020

itswisdomagain Jan 22, 2020

itswisdomagain Jan 22, 2020

itswisdomagain Jan 22, 2020

itswisdomagain Jan 22, 2020

itswisdomagain Jan 22, 2020

itswisdomagain Jan 22, 2020

itswisdomagain Jan 22, 2020

		- name: Install linter
		run: "curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh \| sh -s -- -b $(go env GOPATH)/bin v1.22.2"

	// canUseDir returns true if the program can create the directory
	// canUseDir returns true if the program can create the directory.

multi: fix dcrwallet/errors usage #92

multi: fix dcrwallet/errors usage #92

Conversation

beansgum commented Jan 18, 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

oluwandabira 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

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

Choose a reason for hiding this comment

itswisdomagain 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

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

Modified the f function to print somewhat detailed reports

First test result + log output (TEST PASSED with exit code 0, but should it have?)

Second test result + log output (TEST FAILED with exit code 1, but should it have?)

Other observations:

An interesting edge case

Modified the `f` function to print somewhat detailed reports