Refactor daemon, add more tests #1039

Eric-Warehime · 2022-06-16T00:51:01Z

Summary

First in a series which aims to clean up our commands into something more testable and adds coverage.

Removed the makeOptions call in api_config.go because it was only being used for a single argument, and I've refactored it to take a config.
Moved the variables in daemon.go to a struct, daemonConfig
Added create functions for daemonConfig, and daemonCmd
Moved the bulk of the daemon logic into its own function, runDaemon. In the future we can separate this out further into more modular testable pieces, for now I've left it as one giant runner.
I've begun the process of removing panic and os.Exit calls from this except in the base cobra command. By only passing errors around it becomes more easily testable, we don't have to worry about defer funcs not running which is the reason we have the panic logic, and we are still able to properly exit non-zero when the error reaches the command's Run function.
makeOptions has been refactored to convert between a daemonConfig and api.ExtraOptions.
Added BindFlagSet which is a copy of BindFlags since it makes sense to pass the FlagSet around instead of the command, but I'm not yet able to fully convert the existing function without more changes.

Test Plan

Added various tests to get started on testing--mostly for config variables/params for now. It shows how the new setup can be used to simplify tests and validate specific scenarios.

winder

This is a great start. RIP globals.

config/config.go

cmd/algorand-indexer/daemon.go

cmd/algorand-indexer/daemon_test.go

codecov · 2022-06-16T16:25:54Z

Codecov Report

❗ No coverage uploaded for pull request base (localledger/integration@997719a). Click here to learn what that means.
The diff coverage is n/a.

@@                    Coverage Diff                     @@
##             localledger/integration    #1039   +/-   ##
==========================================================
  Coverage                           ?   60.02%           
==========================================================
  Files                              ?       49           
  Lines                              ?     8731           
  Branches                           ?        0           
==========================================================
  Hits                               ?     5241           
  Misses                             ?     3004           
  Partials                           ?      486

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 997719a...9580434. Read the comment docs.

Eric-Warehime · 2022-06-23T15:53:17Z

@winder @AlgoStephenAkiki @shiqizng Should be ready for another round of reviews whenever you all have a chance. :D

winder

Just a few suggestions. Overall this is really great.

cmd/algorand-indexer/daemon.go

cmd/algorand-indexer/daemon_test.go

winder · 2022-06-24T00:19:56Z

cmd/algorand-indexer/daemon_test.go

+	if !strings.Contains(err.Error(), errorStr) {
+		t.Fatalf("expected error %s, but got %s", errorStr, err.Error())
+	}


You get the idea, same thing for the rest of these.

Suggested change

if !strings.Contains(err.Error(), errorStr) {

t.Fatalf("expected error %s, but got %s", errorStr, err.Error())

}

require.Error(t, err)

require.Contains(t, err.Error(), errorStr)

cmd/algorand-indexer/daemon_test.go

cmd/algorand-indexer/main.go

cmd/algorand-indexer/daemon_test.go

shiqizng

This is great! It looks like we should be able to write integration tests for daemon after this change.

…new-daemon-tests

winder · 2022-06-28T18:41:47Z

cmd/algorand-indexer/daemon.go

-		if indexerDataDir == "" {
-			fmt.Fprint(os.Stderr, "indexer data directory was not provided")
-			panic(exit{1})
+func loadIndexerConfig(indexerDataDir string, configFile string) error {


@AlgoStephenAkiki this function looks correct to me. Is anything needed for the BindFlags still?

We would still need to bind the flags yes.

winder · 2022-06-28T18:42:33Z

cmd/algorand-indexer/daemon.go

-			defer configs.Close()
-			err = viper.ReadConfig(configs)
+	// Detect the various auto-loading configs from data directory
+	if err = loadIndexerConfig(daemonConfig.indexerDataDir, daemonConfig.configFile); err != nil {


@AlgoStephenAkiki if we move this ahead of the BindFlagSet call, can we avoid calling that function twice?

I don't think so since the problem is that we have a circular dependency on the logger inside the loadIndexerConfig function (the logger needs to be configured from configuration and the function uses the logger but also can re-load the configuration).

winder

Looks good, just want Stephen to double check the config file loading.

cmd/algorand-indexer/daemon_test.go

AlgoStephenAkiki · 2022-06-29T17:53:23Z

cmd/algorand-indexer/daemon.go

+		logger.WithError(err).Errorf("invalid config file (%s): %v", viper.ConfigFileUsed(), err)
+		return err
+	}
+	logger.Infof("Using configuration file: %s\n", resolvedConfigPath)


After this log statement we should call BindSetFlags() to make sure we get everything set.

* Local Ledger (#1011) * integrate block processor * Local Ledger Deployment (#1013) * add simple local ledger migration * add deleted opts * fast catchup (#1023) * add fast catchup * Localledger merge (#1036) * return empty lists from fetchApplications and fetchAppLocalStates (#1010) * Update model to converge with algod (#1005) * New Feature: Adds Data Directory Support (#1012) - Updates the go-algorand submodule hash to point to rel/beta - Moves the cpu profiling file, pid file and indexer configuration file to be options of only the daemon sub-command - Changes os.Exit() to be a panic with a special handler. This is so that defer's are handled instead of being ignored. - Detects auto-loading configuration files in the data directory and issues errors if equivalent command line arguments are supplied. - Updates the README with instructions on how to use the auto-loading configuration files and the data directory. * Update mockery version Co-authored-by: erer1243 <[email protected]> Co-authored-by: AlgoStephenAkiki <[email protected]> * recovery scenario (#1024) * handle ledger recovery scenario * refactor create genesis block (#1026) * refactor create genesis block * Adds Local Ledger Readme (#1035) * Adds Local Ledger Readme Resolves #4109 Starts Readme docs * Update docs/LocalLedger.md Co-authored-by: Will Winder <[email protected]> * Update docs/LocalLedger.md Co-authored-by: Will Winder <[email protected]> * Update docs/LocalLedger.md Co-authored-by: Will Winder <[email protected]> * Removed troubleshooting section Co-authored-by: Will Winder <[email protected]> * update ledger file path and migration (#1042) * LocalLedger Refactoring + Catchpoint Service (#1049) Part 1 cleanup genesis file access. put node catchup into a function that can be swapped out with the catchup service. pass the indexer logger into the block processor. move open ledger into a util function, and move the initial state util function into a new ledger util file. add initial catchupservice implementation. move ledger init from daemon.go to constructor. Merge multiple read genesis functions. Part 2 Merge local_ledger migration package into blockprocessor. Rename Migration to Initialize Use logger in catchup service catchup Part 3 Update submodule and use NewWrappedLogger. Make util.CreateInitState private * build: merge develop into localledger/integration (#1062) * Ledger init status (#1058) * Generate an error if the catchpoint is not valid for initialization. (#1075) * Use main logger in handler and fetcher. (#1077) * Switch from fullNode catchup to catchpoint catchup service. (#1076) * Refactor daemon, add more tests (#1039) Refactors daemon cmd into separate, testable pieces. * Merge develop into localledger/integration (#1083) * Misc Local Ledger cleanup (#1086) * Update processor/blockprocessor/initialize.go Co-authored-by: Zeph Grunschlag <[email protected]> * commit * fix function call args * RFC-0001: Rfc 0001 impl (#1069) Adds an Exporter interface and a noop exporter implementation with factory methods for construction * Fix test errors * Add/fix tests * Add postgresql_exporter tests * Update config loading * Change BlockExportData to pointers * Move and rename ExportData * Add Empty func to BlockData * Add comment Co-authored-by: shiqizng <[email protected]> Co-authored-by: [email protected] <[email protected]> Co-authored-by: erer1243 <[email protected]> Co-authored-by: AlgoStephenAkiki <[email protected]> Co-authored-by: Will Winder <[email protected]> Co-authored-by: Zeph Grunschlag <[email protected]>

Eric-Warehime force-pushed the new-daemon-tests branch from 2ef3652 to ce855b1 Compare June 16, 2022 00:51

winder reviewed Jun 16, 2022

View reviewed changes

config/config.go Show resolved Hide resolved

cmd/algorand-indexer/daemon.go Outdated Show resolved Hide resolved

winder reviewed Jun 16, 2022

View reviewed changes

cmd/algorand-indexer/daemon_test.go Outdated Show resolved Hide resolved

Eric-Warehime marked this pull request as ready for review June 16, 2022 19:27

Eric-Warehime requested review from AlgoStephenAkiki, shiqizng and winder June 16, 2022 19:27

Eric-Warehime added 8 commits June 21, 2022 21:18

Refactor daemon, add more tests

b5a3022

Fix formatting errors

30cb783

Remove BindFlags in favor of BindFlagSet

58c4a45

Remove daemonCmd global, directly set flags in tests

f5d24b4

Add more tests, break apart runDaemon

08a74d6

Add tests, move pid file creation to func

2f67e03

Refactor daemon

24e4621

Fix CI failures

b762879

Eric-Warehime force-pushed the new-daemon-tests branch from 3efd44e to b762879 Compare June 22, 2022 15:26

Eric-Warehime added 2 commits June 22, 2022 08:29

Merge branch 'localledger/integration' into new-daemon-tests

56c4804

Remove unused import

5f4cd1a

winder suggested changes Jun 24, 2022

View reviewed changes

shiqizng reviewed Jun 24, 2022

View reviewed changes

Eric-Warehime added 4 commits June 27, 2022 10:06

Merge remote-tracking branch 'upstream/localledger/integration' into …

473a562

…new-daemon-tests

Address PR comments

0e73d53

Add comment to address lint failure

3b093bf

Remove no config error

5fd2e7d

Eric-Warehime requested review from shiqizng and winder June 27, 2022 22:06

winder reviewed Jun 28, 2022

View reviewed changes

winder approved these changes Jun 28, 2022

View reviewed changes

shiqizng reviewed Jun 28, 2022

View reviewed changes

cmd/algorand-indexer/daemon_test.go Outdated Show resolved Hide resolved

Change require to assert

c93898b

shiqizng approved these changes Jun 28, 2022

View reviewed changes

AlgoStephenAkiki reviewed Jun 29, 2022

View reviewed changes

Eric-Warehime added 2 commits June 29, 2022 11:05

Add BindFlagSet after loading indexer config

76aba4f

Merge branch 'localledger/integration' into new-daemon-tests

4556661

AlgoStephenAkiki approved these changes Jun 29, 2022

View reviewed changes

Eric-Warehime added 2 commits June 29, 2022 11:16

Fix lint error from merged code

f533612

Fix merge errors

9580434

Eric-Warehime merged commit f3f1177 into algorand:localledger/integration Jun 29, 2022

Eric-Warehime deleted the new-daemon-tests branch June 29, 2022 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor daemon, add more tests #1039

Refactor daemon, add more tests #1039

Eric-Warehime commented Jun 16, 2022

winder left a comment

codecov bot commented Jun 16, 2022 •

edited

Loading

Eric-Warehime commented Jun 23, 2022

winder left a comment

winder Jun 24, 2022

shiqizng left a comment

winder Jun 28, 2022

AlgoStephenAkiki Jun 29, 2022

winder Jun 28, 2022

AlgoStephenAkiki Jun 29, 2022

winder left a comment

AlgoStephenAkiki Jun 29, 2022

Refactor daemon, add more tests #1039

Refactor daemon, add more tests #1039

Conversation

Eric-Warehime commented Jun 16, 2022

Summary

Test Plan

winder left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 16, 2022 • edited Loading

Codecov Report

Eric-Warehime commented Jun 23, 2022

winder left a comment

Choose a reason for hiding this comment

winder Jun 24, 2022

Choose a reason for hiding this comment

shiqizng left a comment

Choose a reason for hiding this comment

winder Jun 28, 2022

Choose a reason for hiding this comment

AlgoStephenAkiki Jun 29, 2022

Choose a reason for hiding this comment

winder Jun 28, 2022

Choose a reason for hiding this comment

AlgoStephenAkiki Jun 29, 2022

Choose a reason for hiding this comment

winder left a comment

Choose a reason for hiding this comment

AlgoStephenAkiki Jun 29, 2022

Choose a reason for hiding this comment

codecov bot commented Jun 16, 2022 •

edited

Loading