Skip to content

Make account changeonlinestatus online flag optional#1170

Merged
tsachiherman merged 4 commits intoalgorand:masterfrom
egieseke:eric/make-account-online-flag-optional
Jun 20, 2020
Merged

Make account changeonlinestatus online flag optional#1170
tsachiherman merged 4 commits intoalgorand:masterfrom
egieseke:eric/make-account-online-flag-optional

Conversation

@egieseke
Copy link
Copy Markdown
Contributor

Summary

Since the online option in the goal account changeonlinestatus defaults to true, updated the command so that the online option is not required.

Test Plan

Ran make test and make integration.

Copy link
Copy Markdown
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

The change itself is fine, but it's missing the corresponding unit test.
The unit test should be one that fails without the goal change and passes with.
( i.e. the unit test should cover the correct behavior of goal utility in regards to this command )

@egieseke
Copy link
Copy Markdown
Contributor Author

egieseke commented Jun 15, 2020

@tsachiherman There is an integration test for this today, which uncovered the issue as part of the Jenkins/Mule integration work. The test is: test/e2e-go/cli/goal/expect/listExpiredParticipationKeyTest.exp

@tsachiherman
Copy link
Copy Markdown
Contributor

@tsachiherman There is an integration test for this today, which uncovered the issue as part of the Jenkins/Mule integration work. The test is: test/e2e-go/cli/goal/expect/listExpiredParticipationKeyTest.exp

Yes, but this integration test is clearly broken. Otherwise, it would have failed before.
( i.e. it should have consistently failed without the changes you've made here. this clearly did not happen )

For this PR to be complete, it should demonstrate the not passing the "online" flag at all is equivalent to passing a "online true", as well as that the "online false" is working as expected.

@tsachiherman tsachiherman dismissed their stale review June 20, 2020 17:27

I'm dismissing my change requests since my upcoming change would include a fix for this issue.

@tsachiherman tsachiherman merged commit eeedd3d into algorand:master Jun 20, 2020
@egieseke egieseke deleted the eric/make-account-online-flag-optional branch June 24, 2020 01:39
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