test: implement index manager tests #982

frrist · 2022-06-09T03:28:16Z

also found and fixed deadlock on fatal errors

codecov-commenter · 2022-06-09T03:36:37Z

Codecov Report

Merging #982 (e9515ea) into master (0464ef6) will increase coverage by 1.2%.
The diff coverage is 83.3%.

@@           Coverage Diff            @@
##           master    #982     +/-   ##
========================================
+ Coverage    34.0%   35.2%   +1.2%     
========================================
  Files          30      31      +1     
  Lines        2020    2087     +67     
========================================
+ Hits          687     736     +49     
- Misses       1256    1266     +10     
- Partials       77      85      +8

frrist · 2022-06-09T03:36:40Z

chain/indexer/integrated/manager.go

-			return false, fatal
-		default:
+	success := atomic.NewBool(true)
+	grp, ctx := errgroup.WithContext(ctx)


introduction of errgroup fixes deadlock by allowing both channels to be read from simultaneously. Prior to this taskErrors was only read when a taskResults was received. If there wasn't a taskResult then the tipset indexer deadlocked writing to the taskErrors channel since nothing was reading from it.

frrist · 2022-06-09T03:37:32Z

chain/indexer/integrated/tipset/tipset.go

@@ -331,11 +334,12 @@ func (t *TipSetIndexer) TipSet(ctx context.Context, ts *types.TipSet) (chan *Res

 	var (
 		outCh = make(chan *Result, len(stateResults))
-		errCh = make(chan error)
+		errCh = make(chan error, 1)


buffered channel to ensure writes never block

frrist · 2022-06-09T03:38:00Z

chain/indexer/integrated/tipset/tipset.go

 	)
 	go func() {
 		defer func() {
 			close(outCh)
+			close(errCh)


close the channel signaling to consumers in the manager to exit.

frrist · 2022-06-09T03:38:22Z

lens/lily/impl.go

-func (m *LilyNodeAPI) LogList(ctx context.Context) ([]string, error) {
+func (m *LilyNodeAPI) LogList(_ context.Context) ([]string, error) {
 	return logging.GetSubsystems(), nil
 }

-func (m *LilyNodeAPI) LogSetLevel(ctx context.Context, subsystem, level string) error {
+func (m *LilyNodeAPI) LogSetLevel(_ context.Context, subsystem, level string) error {
 	return logging.SetLogLevel(subsystem, level)
 }

-func (m *LilyNodeAPI) LogSetLevelRegex(ctx context.Context, regex, level string) error {
+func (m *LilyNodeAPI) LogSetLevelRegex(_ context.Context, regex, level string) error {
 	return logging.SetLogLevelRegex(regex, level)
 }

-func (m *LilyNodeAPI) Shutdown(ctx context.Context) error {
+func (m *LilyNodeAPI) Shutdown(_ context.Context) error {


frrist · 2022-06-09T03:38:28Z

lens/lily/impl.go

-func (l *LogQueryHook) AfterQuery(ctx context.Context, event *pg.QueryEvent) error {
+func (l *LogQueryHook) AfterQuery(_ context.Context, _ *pg.QueryEvent) error {
 	return nil


more linting

frrist · 2022-06-09T03:39:03Z

chain/indexer/integrated/manager_test.go

@@ -0,0 +1,562 @@
+package integrated


unit test the indexer manager with mocks

- fix deadlock on fatal errors

placer14

Tests look wonderful! Thanks for comments, as well! 🍻

placer14 · 2022-06-14T18:30:18Z

chain/indexer/integrated/manager.go

@@ -54,7 +60,7 @@ func NewManager(api tasks.DataSource, strg model.Storage, name string, opts ...M
 		opt(im)
 	}

-	im.indexBuilder = NewBuilder(api, name)
+	im.indexBuilder = idxBuilder

 	if im.exporter == nil {
 		im.exporter = indexer.NewModelExporter(name)


The stuttering on name in the function signature suggests you could collapse it into the builder. Making the builder responsible for the name gives you a cleaner signature.

func NewManager(strg model.Storage, idxBuilder tipset.IndexerBuilder, opts ...ManagerOpt) (*Manager, error)

Suggested change

im.exporter = indexer.NewModelExporter(name)

im.exporter = indexer.NewModelExporter(idxBuilder.Name())

ohh god idea!

placer14 · 2022-06-14T18:37:50Z

chain/indexer/integrated/manager.go


-	// synchronously export extracted data and its report. If datas at this height are currently being persisted this method will block to avoid deadlocking the database.
-	if err := i.exporter.ExportResult(ctx, i.storage, int64(ts.Height()), modelResults); err != nil {
+		if len(modelResults) > 0 {


If you put this check inside ExportResult, then the dev doesn't need to worry about this case before consuming it. According to the godoc there, you could return a nil error and not break the semantics.

placer14 · 2022-06-14T18:42:16Z

chain/indexer/integrated/manager_test.go

+	require.True(t, success)
+}
+
+func TestManagerStatusOKAndError(t *testing.T) {


I would like to see this test name in the failure output and understand what is being protected. Something like:

Suggested change

func TestManagerStatusOKAndError(t *testing.T) {

func TestManagerResultsContainingErrorReturnsError(t *testing.T) {

placer14 · 2022-06-14T18:42:48Z

chain/indexer/integrated/manager_test.go

+
+}
+
+func TestManagerStatusOKAndSkip(t *testing.T) {


Suggested change

func TestManagerStatusOKAndSkip(t *testing.T) {

func TestManagerResultsContainingSkipReturnsError(t *testing.T) {

placer14 · 2022-06-14T18:44:15Z

chain/indexer/integrated/manager_test.go

+	success, err := manager.TipSet(ctx, ts1)
+	require.NoError(t, err)
+	require.False(t, success)
+}


Maybe this is a repeat of the test at https://github.com/filecoin-project/lily/pull/982/files#diff-0df3028ac9493f131c71862e7a8dd54d2e09a4ed31205f2a0a2fc09d31f1e976R149 (TestManagerStatusOKAndError)?

placer14 · 2022-06-14T18:44:52Z

chain/indexer/integrated/manager_test.go

+	success, err := manager.TipSet(ctx, ts1)
+	require.Error(t, err)
+	require.False(t, success)
+}


Repeat of https://github.com/filecoin-project/lily/pull/982/files#diff-0df3028ac9493f131c71862e7a8dd54d2e09a4ed31205f2a0a2fc09d31f1e976R239?

Could you tell or show me what is being repeated? A lot of the setup is the same, but the tests validate different things.

Sorry, I started to recontextualize what I was thinking here (I should've been more descriptive) and abandoned for other tasks. It's not a big deal if there's double coverage, if at all. It just seemed there was overlapping coverage in these examples. Nit only.

placer14 · 2022-06-14T18:58:20Z

chain/indexer/integrated/manager_test.go

+	require.False(t, success)
+}
+
+func TestManagerFatalErrorAndOkReport(t *testing.T) {


Suggested change

func TestManagerFatalErrorAndOkReport(t *testing.T) {

func TestManagerFatalErrorDoesntBlockOkReport(t *testing.T) {

frrist self-assigned this Jun 9, 2022

frrist requested review from hsanjuan, kasteph and placer14 June 9, 2022 03:28

frrist force-pushed the frrist/manager-test branch from 96bddd2 to e1d6c42 Compare June 9, 2022 03:33

frrist commented Jun 9, 2022

View reviewed changes

chain/indexer/integrated/manager_test.go

@@ -0,0 +1,562 @@

package integrated

Copy link

Member Author

frrist Jun 9, 2022

Choose a reason for hiding this comment

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

unit test the indexer manager with mocks

test: implement index manager tests

72c7ff3

- fix deadlock on fatal errors

frrist force-pushed the frrist/manager-test branch from e1d6c42 to 72c7ff3 Compare June 9, 2022 03:39

placer14 approved these changes Jun 14, 2022

View reviewed changes

address feedback

e9515ea

frrist force-pushed the frrist/manager-test branch from f63b805 to e9515ea Compare June 16, 2022 15:42

frrist merged commit 56fcf12 into master Jun 16, 2022

frrist deleted the frrist/manager-test branch June 16, 2022 22:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: implement index manager tests #982

test: implement index manager tests #982

frrist commented Jun 9, 2022 •

edited

Loading

codecov-commenter commented Jun 9, 2022 •

edited

Loading

frrist Jun 9, 2022

frrist Jun 9, 2022

frrist Jun 9, 2022

frrist Jun 9, 2022

frrist Jun 9, 2022

frrist Jun 9, 2022

placer14 left a comment

placer14 Jun 14, 2022

frrist Jun 15, 2022

placer14 Jun 14, 2022

placer14 Jun 14, 2022

placer14 Jun 14, 2022

placer14 Jun 14, 2022 •

edited

Loading

placer14 Jun 14, 2022

frrist Jun 15, 2022 •

edited

Loading

placer14 Jun 22, 2022

placer14 Jun 14, 2022

	im.exporter = indexer.NewModelExporter(name)
	im.exporter = indexer.NewModelExporter(idxBuilder.Name())

	func TestManagerStatusOKAndError(t *testing.T) {
	func TestManagerResultsContainingErrorReturnsError(t *testing.T) {

	func TestManagerStatusOKAndSkip(t *testing.T) {
	func TestManagerResultsContainingSkipReturnsError(t *testing.T) {

	func TestManagerFatalErrorAndOkReport(t *testing.T) {
	func TestManagerFatalErrorDoesntBlockOkReport(t *testing.T) {

test: implement index manager tests #982

test: implement index manager tests #982

Conversation

frrist commented Jun 9, 2022 • edited Loading

codecov-commenter commented Jun 9, 2022 • edited Loading

Codecov Report

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

placer14 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

placer14 Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frrist Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frrist commented Jun 9, 2022 •

edited

Loading

codecov-commenter commented Jun 9, 2022 •

edited

Loading

placer14 Jun 14, 2022 •

edited

Loading

frrist Jun 15, 2022 •

edited

Loading