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

Use this in tests #1200

Closed
nventuro opened this issue Aug 13, 2018 · 3 comments · Fixed by #1380
Closed

Use this in tests #1200

nventuro opened this issue Aug 13, 2018 · 3 comments · Fixed by #1380
Labels
good first issue Low hanging fruit for new contributors to get involved! tests Test suite and helpers.
Milestone

Comments

@nventuro
Copy link
Contributor

Some tests still have global(ish) state, we should move towards storing those in this.

Part of #1091.

@nventuro nventuro added good first issue Low hanging fruit for new contributors to get involved! kind:improvement tests Test suite and helpers. labels Aug 13, 2018
@nventuro nventuro mentioned this issue Aug 13, 2018
9 tasks
@nventuro nventuro added this to the v2.1 milestone Aug 24, 2018
@Aniket-Engg
Copy link
Contributor

Aniket-Engg commented Oct 3, 2018

@nventuro I think most of the tests are following this. Can you please point to the files lacking this?

@nventuro
Copy link
Contributor Author

nventuro commented Oct 3, 2018

To clarify, this doesn't refer to configuration variables (e.g. const cap = ether(20);), but to things like

let token;
beforeEach(async function () {
  token = await Token.new();
});

examples/SimpleToken is guilty of this, as is MerkleProof, DetailedERC20, and ReentracnyGuard. There may be more examples of this, but I'm not sure.

Aniket-Engg added a commit to Aniket-Engg/zeppelin-solidity that referenced this issue Oct 4, 2018
@Aniket-Engg Aniket-Engg mentioned this issue Oct 4, 2018
4 tasks
@Aniket-Engg
Copy link
Contributor

Aniket-Engg commented Oct 4, 2018

I checked each test file. Changes were required only in the files mentioned by @nventuro . Still if anyone finds that I missed something, Please add here.

nventuro pushed a commit that referenced this issue Oct 4, 2018
* signing prefix added

* Minor improvement

* Tests changed

* Successfully tested

* Minor improvements

* Minor improvements

* Revert "Dangling commas are now required. (#1359)"

This reverts commit a688977.

* updates

* fixes #1200

* suggested change
come-maiz pushed a commit that referenced this issue Oct 21, 2018
* signing prefix added

* Minor improvement

* Tests changed

* Successfully tested

* Minor improvements

* Minor improvements

* Revert "Dangling commas are now required. (#1359)"

This reverts commit a688977.

* updates

* fixes #1200

* suggested change

(cherry picked from commit b41b125)
@come-maiz come-maiz modified the milestones: v2.1, v2.0 Oct 21, 2018
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.

3 participants