Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename 'anyone' account in tests to something else #1357

Closed
nventuro opened this issue Sep 26, 2018 · 11 comments · Fixed by #1718
Closed

Rename 'anyone' account in tests to something else #1357

nventuro opened this issue Sep 26, 2018 · 11 comments · Fixed by #1718
Labels
good first issue Low hanging fruit for new contributors to get involved! tests Test suite and helpers.

Comments

@nventuro
Copy link
Contributor

When testing access control related functionality, we usually have an anyone account that represents an account with no special permissions, e.g.:

it('reverts when anyone attempts to self-destruct', async function () {
  await assertRevert(this.contract.destroy({ from: anyone });
});

anyone is probably not the best name we can find, we should find something that better conveys the meaning of 'anyone else', 'any random account', etc.

@nventuro nventuro added kind:improvement tests Test suite and helpers. labels Sep 26, 2018
@yosriady
Copy link

How about stranger?

@Aniket-Engg
Copy link
Contributor

what about guest?

@frangio
Copy link
Contributor

frangio commented Oct 1, 2018

I think other is better than anyone because it conveys that it is a different account from all the other ones used in the same test.

@nventuro
Copy link
Contributor Author

nventuro commented Oct 2, 2018

Hmm, I like other better than the other proposals, I think it could work.

@mswezey23
Copy link
Contributor

My vote is on other as well.

@nventuro nventuro added good first issue Low hanging fruit for new contributors to get involved! and removed improvement labels Apr 8, 2019
@ckshei
Copy link
Contributor

ckshei commented Apr 9, 2019

I'd like to try and help work on this

@ckshei
Copy link
Contributor

ckshei commented Apr 9, 2019

I'll probably need a bit of guidance though - are we looking to change all references in anyone to "other" in the tests, or only in some specific instances, or only in the description?

@nventuro
Copy link
Contributor Author

I think just the account name: test descriptions to go 'tokens can be transferred to anyone' are actually correct (assuming anyone is no-none 😛).

@ckshei
Copy link
Contributor

ckshei commented Apr 10, 2019

👍 got it. @nventuro, am I changing each instance of from: anyone to from: other? Or are some of the "anyone's" valid in the tests?

In other words, I'm only changing this:

  it('can be finalized by anyone after ending', async function () {
    await time.increaseTo(this.afterClosingTime);
    await this.crowdsale.finalize({ from: anyone });
  });

to

  it('can be finalized by anyone after ending', async function () {
    await time.increaseTo(this.afterClosingTime);
    await this.crowdsale.finalize({ from: other });
  });

@nventuro
Copy link
Contributor Author

Correct! I don't think we want to leave any instance of anyone: the core issue is that 'anyone' shouldn't be an account name (because it belongs to 'someone'). 'other' implies that that account isn't special (and represents 'anyone', which is why we leave it in the test descriptions 😛).

@ckshei
Copy link
Contributor

ckshei commented Apr 11, 2019

Got it. @nventuro - You can see my first attempt here: #1718

I changed all the references of "anyone" in the code to "other" and I'm pretty confident I didn't miss any. Just not sure if I might have done a few that weren't meant to have changed. I think I noticed one instance where "other" was declared but never actually used. I could remove that if you'd like.

nventuro pushed a commit that referenced this issue Apr 11, 2019
* replacing all instances of from: anyone with from: other

* replacing all instances of from: anyone with from: other

* replacing all instances of from: anyone with from: other

* changing anyone to other

* changing anyone to other
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Low hanging fruit for new contributors to get involved! tests Test suite and helpers.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants