Skip to content

lnutils: add methods ForEach, Len and LoadOrStore to SyncMap#7563

Merged
Roasbeef merged 2 commits intolightningnetwork:masterfrom
yyforyongyu:syncmap-methods
Apr 11, 2023
Merged

lnutils: add methods ForEach, Len and LoadOrStore to SyncMap#7563
Roasbeef merged 2 commits intolightningnetwork:masterfrom
yyforyongyu:syncmap-methods

Conversation

@yyforyongyu
Copy link
Copy Markdown
Member

Add more methods to SyncMap, which are used in multiple PRs now.

@yyforyongyu yyforyongyu added this to the v0.16.1 milestone Apr 3, 2023
@yyforyongyu yyforyongyu self-assigned this Apr 3, 2023
Copy link
Copy Markdown
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Nice additions!

I asked Copilot to generate some tests for this with the following prompt:

Given the go program below, can you generate some unit tests in Go to properly exercises the implementation? This uses Go 1.18 so it has generics, also known as type parameters. The tests should be exhaustive (reach 100% unit test coverage). Ideally the test is also able to add randomized inputs, and also test invariants using the testing/quick package.

It produced:

package lnutils

import (
	"fmt"
	"testing"
	"testing/quick"
)

func TestSyncMap(t *testing.T) {
	t.Run("Store and Load", func(t *testing.T) {
		sm := &SyncMap[string, int]{}
		key := "testKey"
		value := 123

		sm.Store(key, value)

		actual, ok := sm.Load(key)
		if !ok {
			t.Error("expected item to be found")
		}
		if actual != value {
			t.Errorf("expected %v, got %v", value, actual)
		}
	})

	t.Run("Load not found", func(t *testing.T) {
		sm := &SyncMap[string, int]{}

		actual, ok := sm.Load("nonExistent")
		if ok {
			t.Error("expected item to not be found")
		}
		if actual != 0 {
			t.Errorf("expected 0, got %v", actual)
		}
	})

	t.Run("Load type assertion fails", func(t *testing.T) {
		sm := &SyncMap[string, int]{}
		key := "testKey"
		value := "not an int"
		sm.Store(key, value)

		actual, ok := sm.Load(key)
		if ok {
			t.Error("expected type assertion to fail")
		}
		if actual != 0 {
			t.Errorf("expected 0, got %v", actual)
		}
	})

	t.Run("Delete", func(t *testing.T) {
		sm := &SyncMap[string, int]{}
		key := "testKey"
		value := 123
		sm.Store(key, value)

		sm.Delete(key)

		actual, ok := sm.Load(key)
		if ok {
			t.Error("expected item to be deleted")
		}
		if actual != 0 {
			t.Errorf("expected 0, got %v", actual)
		}
	})

	t.Run("LoadAndDelete", func(t *testing.T) {
		sm := &SyncMap[string, int]{}
		key := "testKey"
		value := 123
		sm.Store(key, value)

		actual, ok := sm.LoadAndDelete(key)
		if !ok {
			t.Error("expected item to be found and deleted")
		}
		if actual != value {
			t.Errorf("expected %v, got %v", value, actual)
		}

		_, ok = sm.Load(key)
		if ok {
			t.Error("expected item to be deleted")
		}
	})

	t.Run("Range", func(t *testing.T) {
		sm := &SyncMap[string, int]{}
		sm.Store("key1", 1)
		sm.Store("key2", 2)
		sm.Store("key3", 3)

		var count int
		sm.Range(func(key string, value int) bool {
			count += value
			return true
		})

		expected := 6
		if count != expected {
			t.Errorf("expected %v, got %v", expected, count)
		}
	})

	t.Run("ForEach", func(t *testing.T) {
		sm := &SyncMap[string, int]{}
		sm.Store("key1", 1)
		sm.Store("key2", 2)
		sm.Store("key3", 3)

		var count int
		sm.ForEach(func(key string, value int) error {
			count += value
			return nil
		})

		expected := 6
		if count != expected

It got cut off (length limit) then generated some additional tests:

import (
    "strconv"
    "sync"
    "testing"
    "testing/quick"
)

func TestSyncMap_StoreLoad(t *testing.T) {
    m := new(SyncMap[string, int])
    m.Store("one", 1)
    m.Store("two", 2)

    v, ok := m.Load("one")
    if !ok || v != 1 {
        t.Errorf("m.Load(\"one\") = (%v, %v), want (1, true)", v, ok)
    }

    v, ok = m.Load("two")
    if !ok || v != 2 {
        t.Errorf("m.Load(\"two\") = (%v, %v), want (2, true)", v, ok)
    }

    v, ok = m.Load("three")
    if ok || v != 0 {
        t.Errorf("m.Load(\"three\") = (%v, %v), want (0, false)", v, ok)
    }
}

func TestSyncMap_Delete(t *testing.T) {
    m := new(SyncMap[string, int])
    m.Store("one", 1)
    m.Delete("one")

    v, ok := m.Load("one")
    if ok || v != 0 {
        t.Errorf("m.Load(\"one\") = (%v, %v), want (0, false)", v, ok)
    }
}

func TestSyncMap_LoadAndDelete(t *testing.T) {
    m := new(SyncMap[string, int])
    m.Store("one", 1)

    v, ok := m.LoadAndDelete("one")
    if !ok || v != 1 {
        t.Errorf("m.LoadAndDelete(\"one\") = (%v, %v), want (1, true)", v, ok)
    }

    v, ok = m.Load("one")
    if ok || v != 0 {
        t.Errorf("m.Load(\"one\") = (%v, %v), want (0, false)", v, ok)
    }
}

func TestSyncMap_Range(t *testing.T) {
    m := new(SyncMap[string, int])
    m.Store("one", 1)
    m.Store("two", 2)

    var keys []string
    var values []int

    m.Range(func(k string, v int) bool {
        keys = append(keys, k)
        values = append(values, v)

        return true
    })

    if len(keys) != 2 || len(values) != 2 {
        t.Errorf("len(keys)=%d, len(values)=%d, want 2", len(keys), len(values))
    }

    if keys[0] != "one" || keys[1] != "two" {
        t.Errorf("keys=%v, want [\"one\", \"two\"]", keys)
    }

    if values[0] != 1 || values[1] != 2 {
        t.Errorf("values=%v, want [1, 2]", values)
    }
}

func TestSyncMap_ForEach(t *testing.T) {
    m := new(SyncMap[string, int])
    m.Store("one", 1)
    m.Store("two", 2)

    var keys []string
    var values []int

    m.ForEach(func(k string, v int) error {
        keys = append(keys, k)
        values = append(values, v)

        return nil
    })

    if len(keys) != 2 || len(values) != 2 {
        t.Errorf("len(keys

Might make more sense to have it generate them incrementally (for each method).

@Roasbeef
Copy link
Copy Markdown
Member

Roasbeef commented Apr 3, 2023

Asked it to generate a testing/quick test for a single method:

Can you generate a quickcheck/invariant test using the testing/quick package for the method LoadOrStore?
Sure, here's an example of a testing/quick test that could be used to check an invariant property of the LoadOrStore method:

import (
	"testing"
	"testing/quick"
)

func TestSyncMap_LoadOrStore(t *testing.T) {
	f := func(key int, value string) bool {
		m := &SyncMap[int, string]{}
		v, ok := m.LoadOrStore(key, value)
		if ok {
			// If the key already exists, the returned value should match
			// the existing value in the map.
			return v == value
		} else {
			// If the key did not exist before, the returned value should be
			// equal to the input value.
			return v == value && m.Len() == 1
		}
	}
	if err := quick.Check(f, nil); err != nil {
		t.Error(err)
	}
}

This test generates random integer keys and string values and checks two invariant properties of the LoadOrStore method:
If the key already exists in the map, the method should return the existing value and true for the second return value.
If the key did not exist before, the method should store the input value under the given key and return the same value and false for the second return value.
This test uses the testing/quick package to generate random input values and check the function with them.

@yyforyongyu
Copy link
Copy Markdown
Member Author

I asked Copilot to generate some tests for this with the following prompt:

Funny cause the new methods were almost all created by copliot😂

@Roasbeef do you think we should add the autogenerated tests into test file?

@Roasbeef
Copy link
Copy Markdown
Member

Roasbeef commented Apr 6, 2023

Funny cause the new methods were almost all created by copliot😂

/insert spider-man-meme

;)

do you think we should add the autogenerated tests into test file?

Up to you if you want to do it here, or in another PR. I think we should add them in as at least they'll give us some minimal coverage to protect against future regressions.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

TILed this usage, thanks to the 🤖

@yyforyongyu
Copy link
Copy Markdown
Member Author

Added unit tests, mostly generated from 🤖
it started giving low quality code in the end tho(maybe due to it being unpaid😂), had to write them manually.

@yyforyongyu yyforyongyu requested a review from Roasbeef April 6, 2023 03:25
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The autogen-magic, is that copilot or copilot X?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it's chatgpt🤓

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

NIce

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also been experimenting with it to generate code structure/architecture diagrams and also markdown ER diagrams for relational tables. Going pretty well so far (doing it in other repos), would like to apply it to lnd as well.

So far used it to come up with some branch automation stuff: #7577

@Roasbeef Roasbeef changed the base branch from 0-16-1-staging to master April 11, 2023 00:00
@Roasbeef Roasbeef merged commit bb3d344 into lightningnetwork:master Apr 11, 2023
@yyforyongyu yyforyongyu deleted the syncmap-methods branch April 11, 2023 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants