Skip to content

fix: crawl_error detection#57

Merged
guillaumemichel merged 3 commits into
mainfrom
fix/error-crawl-detection
Feb 8, 2024
Merged

fix: crawl_error detection#57
guillaumemichel merged 3 commits into
mainfrom
fix/error-crawl-detection

Conversation

@guillaumemichel
Copy link
Copy Markdown
Collaborator

If all runs of the loop failed, then ErrorBits is a power of two minus one, and we should keep the result.Error. Otherwise, at least one iteration was successful, set result.Error to nil.

@dennis-tra
Copy link
Copy Markdown
Owner

dennis-tra commented Feb 7, 2024

To be on the very safe side, could you extract a function like:

func hasAtLeastOneSuccesfulFindNodeOrBetterAShorterFunctionName(errorBits uint32) bool {
   ...
}

and write a test that passes a few reasonable values into it and asserts that it works correctly?

This would also serve as documentation how the logic is supposed to work

@dennis-tra
Copy link
Copy Markdown
Owner

Great thanks, let's merge!

Just want to share my thoughts. I initially thought of a test that was more focussing on the behaviour than the logic (subtle difference). Something along these lines:

	tests := []struct {
		name      string
		err       error
		errorBits uint32
		want      bool
	}{
		{
			name:      "no err",
			err:       nil,
			errorBits: 0b00000000,
			want:      false,
		},
		{
			name:      "first failed",
			err:       fmt.Errorf("some err"),
			errorBits: 0b00000001,
			want:      true,
		},
		{
			name:      "second failed, first worked",
			err:       fmt.Errorf("some err"),
			errorBits: 0b00000010,
			want:      false,
		},
	}

but this test is totally fine 👍

@guillaumemichel
Copy link
Copy Markdown
Collaborator Author

added the tests as you suggested

@guillaumemichel guillaumemichel merged commit 55a4a2e into main Feb 8, 2024
@guillaumemichel guillaumemichel deleted the fix/error-crawl-detection branch February 8, 2024 07:53
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.

2 participants