Task: Reset config cmd #3496

pocockn · 2022-07-04T13:29:50Z

Description

Step 1: refactor command _configResetCmd into ./ioctl/newcmd/config/config_reset.go
Step 2: create ./ioctl/newcmd/config/config_reset_test.go to add unit test

Fixes #3468

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

make test

* upstream/master: (24 commits) account type with zero init nonce (iotexproject#3387) [api] Separate Server and Server Handler (iotexproject#3485) [ioctl] Build hdwallet derive command line into new ioctl (iotexproject#3418) [ioctl] Build hdwallet create command line into new ioctl (iotexproject#3470) [makefile] add go mod tidy (iotexproject#3471) [api] update chain metrics (iotexproject#3484) remove config.EVMNetworkID() (iotexproject#3460) [filedao] remove checkMasterChainDBFile() (iotexproject#3463) [api] add crashlog (iotexproject#3456) [api] Move generateBlockMeta to grpcserver.go (iotexproject#3303) [ioctl] Build action hash command line into new ioctl (iotexproject#3425) [ioctl] Build hdwallet export command line into new ioctl (iotexproject#3423) [ioctl] Refactor nodereward command in new ioctl (iotexproject#3416) [ioctl] Cleanup TestNewNodeDelegateCmd (iotexproject#3421) [blockchain] Remove BoltDBDaoOption (iotexproject#3465) remove InMemDaoOption (iotexproject#3464) [action] add evm london test (iotexproject#3402) [ioctl] create main for ioctl/newcmd (iotexproject#3296) [ioctl] Build block bucket command line into new ioctl (iotexproject#3386) [ioctl] Build hdwallet import command line into new ioctl (iotexproject#3419) ...

codecov · 2022-07-04T13:49:33Z

Codecov Report

Merging #3496 (5488537) into master (a20e489) will decrease coverage by 0.84%.
The diff coverage is 60.24%.

@@            Coverage Diff             @@
##           master    #3496      +/-   ##
==========================================
- Coverage   75.43%   74.59%   -0.85%     
==========================================
  Files         247      252       +5     
  Lines       22845    23133     +288     
==========================================
+ Hits        17233    17255      +22     
- Misses       4685     4951     +266     
  Partials      927      927

Impacted Files	Coverage Δ
blockchain/blockchain.go	`0.89% <0.00%> (ø)`
config/config.go	`94.06% <ø> (+9.31%)`	⬆️
actpool/config.go	`60.00% <60.00%> (ø)`
blockchain/config.go	`65.78% <65.78%> (ø)`
action/protocol/poll/protocol.go	`68.13% <66.66%> (-0.35%)`	⬇️
actpool/actpool.go	`87.09% <100.00%> (ø)`
blockchain/filedao/testing.go	`59.71% <100.00%> (ø)`
ioctl/client.go	`73.61% <100.00%> (+0.32%)`	⬆️
ioctl/newcmd/config/config_reset.go	`100.00% <100.00%> (ø)`
state/factory/factory.go	`78.91% <100.00%> (ø)`
... and 5 more

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 7b46cdc...5488537. Read the comment docs.

CoderZhi · 2022-07-04T18:42:34Z

ioctl/newcmd/config/config_reset_test.go

+	client.EXPECT().Config().Return(config.Config{})
+	apiServiceClient := mock_iotexapi.NewMockAPIServiceClient(ctrl)
+
+	client.EXPECT().APIServiceClient().Return(apiServiceClient, nil).Times(1)


when is this needed?

CoderZhi · 2022-07-04T18:46:00Z

ioctl/newcmd/config/config_reset_test.go

+	client.EXPECT().APIServiceClient().Return(apiServiceClient, nil).Times(1)
+
+	cmd := NewConfigReset(client)
+	result, err := util.ExecuteCmd(cmd, "reset")


the failure case with returning error is not simulated or covered

unit test for config.reset should be added.

I have added a failure case for the command and a unit test for the reset. I had to pass in the config file name into the command to be able to simulate an error, let me know what you think. Cheers

huof6829 · 2022-07-06T08:05:58Z

ioctl/newcmd/config/config_reset.go

+)
+
+// NewConfigReset resets the config to the default values
+func NewConfigReset(client ioctl.Client, defaultConfigFileName string) *cobra.Command {


delete argument because user can't know file name and path of defaultConfigFileName.

you can get it from client.

add client.ConfigFilePath() in client.go

call it in line23

huof6829 · 2022-07-06T08:13:50Z

ioctl/newcmd/config/config_reset.go

+		Use:   "reset",
+		Short: "Reset config to default",


add Chinese translation like

iotex-core/ioctl/newcmd/node/node.go

Lines 16 to 19 in 99f0663

_nodeCmdShorts = map[config.Language]string{

config.English: "Deal with nodes of IoTeX blockchain",

config.Chinese: "处理IoTeX区块链的节点",

}

then use client.SelectTranslation()

huof6829 · 2022-07-06T08:14:21Z

ioctl/newcmd/config/config_reset.go

+			if err != nil {
+				return errors.Wrap(err, "failed to reset config")
+			}
+			cmd.Print("successfully reset config")


cmd.Println

huof6829 · 2022-07-06T08:17:44Z

ioctl/newcmd/config/config_reset.go

+			info := newInfo(client.Config(), defaultConfigFileName)
+			err := info.reset()


combine in one line: newInfo(client.Config(), defaultConfigFileName).reset()

add unit test TestReset in config_test.go

huof6829 · 2022-07-06T08:30:35Z

ioctl/newcmd/config/config_reset_test.go

+	client.EXPECT().Config().Return(config.Config{}).Times(2)
+
+	t.Run("successful config reset", func(t *testing.T) {
+		cmd := NewConfigReset(client, _defaultConfigFileName)


Don't use the true file in test because it can delete the real file and can't revert. you can create temp dir using t.TempDir(), then create file under it

huof6829 · 2022-07-06T08:34:47Z

ioctl/newcmd/config/config_reset_test.go

+		// use invalid file name to force error
+		cmd := NewConfigReset(client, "\x00")
+		_, err := util.ExecuteCmd(cmd, "reset")
+		require.Error(err)


check err with expected error can be better like require.Contains(err.Error(), "expect error")

huof6829 · 2022-07-06T08:37:56Z

ioctl/newcmd/config/config_reset_test.go

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


put into config_test.go because reset() is in config.go

huof6829 · 2022-07-06T08:40:05Z

ioctl/newcmd/config/config_reset_test.go

+	assert.Equal(t, ".", cfg.Wallet)
+	assert.Equal(t, "", cfg.Endpoint)
+	assert.Equal(t, true, cfg.SecureConnect)
+	assert.Equal(t, "English", cfg.Language)
+	assert.Equal(t, _defaultAnalyserEndpoint, cfg.AnalyserEndpoint)
+	assert.Equal(t, "iotexscan", cfg.Explorer)
+	assert.Equal(t, *new(config.Context), cfg.DefaultAccount)


require.Equal

huof6829 · 2022-07-06T08:53:31Z

ioctl/newcmd/config/config_reset_test.go

+	config.DefaultConfigFile = _defaultConfigFileName
+	cfg, err := config.LoadConfig()


Don't use global because it may modify the true config data.

use info.loadConfig() and check data with set before by line43~49. I think this should put into TestLoadConfig. it's just unit test.

…o task/config-reset * 'task/config-reset' of github.com:pocockn/iotex-core: [consensus] remove config.EVMNetworkID() (iotexproject#3491)

huof6829 · 2022-07-06T12:02:48Z

ioctl/newcmd/config/config_test.go

+	config.DefaultConfigFile = _defaultConfigFileName
+	cfg, err := config.LoadConfig()


Don't use global because it may modify the true config data.
use info.loadConfig() and check data with set data lin40~46. I think this can put into TestLoadConfig. it's just unit test.

huof6829 · 2022-07-06T12:04:22Z

ioctl/newcmd/config/config_reset_test.go

+		require.NoError(err)
+		require.Contains(result, "successfully reset config")
+
+		defer testutil.CleanupPath("config.default")


👍 Thanks for the review, I've made the changes

👍 Thanks for the review, I've made the changes

Welcome! 👍

huof6829 · 2022-07-07T11:07:49Z

ioctl/newcmd/config/config_reset.go

+)
+
+// NewConfigReset resets the config to the default values
+func NewConfigReset(client ioctl.Client) *cobra.Command {


use NewConfigResetCmd to be consistent with others

* upstream/master: (28 commits) [ioctl] Incorrect conversion between integer types (iotexproject#3522) [action] fix incorrect conversion between integer types (iotexproject#3545) [test] fix TestLoadBlockchainfromDB (iotexproject#3521) [ioctl] correct Chinese usage message (iotexproject#3510) fix err not hanled (iotexproject#3509) add ReadHeaderTimeout (iotexproject#3539) [iotcl] Reset config cmd (iotexproject#3496) Update gosec.yaml Update ci.yaml Update analysis.yaml Delete gosec.yaml.bak Create gosec.yaml [config] move config.ActPool to actpool package refactor (iotexproject#3514) Update ci.yaml Update analysis.yaml Update analysis.yaml [config] move config.Chain to blockchain package (iotexproject#3511) remove circleci (iotexproject#3498) [ioctl] Build block bucketlist command line into new ioctl (iotexproject#3469) [config] remove EVMNetworkID() and SetEVMNetworkID() (iotexproject#3503) ...

pocockn and others added 6 commits June 9, 2022 15:18

Task: Return error from keep alive methods within httputil pkg

996573e

Merge branch 'master' into master

3b3504e

Merge branch 'master' into master

e705bba

Merge branch 'master' into master

3a272b4

Task: Add reset config cmd

322086f

pocockn requested a review from a team as a code owner July 4, 2022 13:29

Add os remove to the test as clean up

3564b8d

CoderZhi reviewed Jul 4, 2022

View reviewed changes

pocockn added 4 commits July 5, 2022 14:41

Task: Add error test and unit test for reseting config

7337d73

Remove file

fc5ec25

Clean up: import names

47109c3

Merge branch 'master' into task/config-reset

a17954e

huof6829 reviewed Jul 6, 2022

View reviewed changes

pocockn added 3 commits July 6, 2022 13:20

Add method to fetch config file path

3912021

Merge branch 'task/config-reset' of github.com:pocockn/iotex-core int…

d77eaaa

…o task/config-reset * 'task/config-reset' of github.com:pocockn/iotex-core: [consensus] remove config.EVMNetworkID() (iotexproject#3491)

Use t.TempDir

b271ba2

huof6829 reviewed Jul 6, 2022

View reviewed changes

pocockn added 4 commits July 6, 2022 15:41

Avoid using the default config file path in tests

66344f9

Merge branch 'master' into task/config-reset

14854b6

Clean up: Remove unused import

36154e8

Task: Add config file path unit test

9c51b9d

huof6829 reviewed Jul 7, 2022

View reviewed changes

huof6829 approved these changes Jul 7, 2022

View reviewed changes

Rename command to make it consistent

0531b01

Liuhaai approved these changes Jul 11, 2022

View reviewed changes

Merge branch 'master' into task/config-reset

d410004

Liuhaai added 2 commits July 11, 2022 22:56

Merge branch 'master' into task/config-reset

3ed547b

Merge branch 'master' into task/config-reset

5488537

Liuhaai merged commit 58b89f7 into iotexproject:master Jul 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task: Reset config cmd #3496

Task: Reset config cmd #3496

pocockn commented Jul 4, 2022

codecov bot commented Jul 4, 2022 •

edited

Loading

CoderZhi Jul 4, 2022

pocockn Jul 5, 2022

CoderZhi Jul 4, 2022

pocockn Jul 5, 2022

huof6829 Jul 6, 2022

huof6829 Jul 6, 2022

huof6829 Jul 6, 2022

huof6829 Jul 6, 2022

huof6829 Jul 6, 2022

huof6829 Jul 6, 2022

huof6829 Jul 6, 2022

huof6829 Jul 6, 2022

huof6829 Jul 6, 2022

huof6829 Jul 6, 2022

huof6829 Jul 6, 2022

huof6829 Jul 6, 2022

pocockn Jul 6, 2022

huof6829 Jul 7, 2022

huof6829 Jul 7, 2022

	_nodeCmdShorts = map[config.Language]string{
	config.English: "Deal with nodes of IoTeX blockchain",
	config.Chinese: "处理IoTeX区块链的节点",
	}

		info := newInfo(client.Config(), defaultConfigFileName)
		err := info.reset()

		config.DefaultConfigFile = _defaultConfigFileName
		cfg, err := config.LoadConfig()

Task: Reset config cmd #3496

Task: Reset config cmd #3496

Conversation

pocockn commented Jul 4, 2022

Description

Type of change

How Has This Been Tested?

codecov bot commented Jul 4, 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

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

codecov bot commented Jul 4, 2022 •

edited

Loading