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

Fixed require statement in setDistribution and improved testing coverage #1290

Closed

Conversation

PeterMPhillips
Copy link
Member

Also did npm link on the tps-helpers package which may have caused the errors @topocount was seeing

@PeterMPhillips PeterMPhillips requested review from a team as code owners October 3, 2019 22:10
@ghost ghost requested review from chadoh, ottodevs and Quazia and removed request for a team October 3, 2019 22:10
Copy link
Collaborator

@chadoh chadoh left a comment

Choose a reason for hiding this comment

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

One note. I think this should probably change, but I'm not 100% sure, so I'm not marking as "request changes." I'm also not marking as "approved" because I want someone with more contract experience to sign off on this. @topocount @Quazia @ottodevs, can one of you take a look, please?

apps/allocations/package.json Show resolved Hide resolved
@ottodevs
Copy link
Member

ottodevs commented Oct 4, 2019

Just a note: web3/allocations is already merged, please don't target that branch again.
I change this to target dev instead.

Edit: After clarifying with @topocount , let's target web3/allocations again

@ottodevs ottodevs changed the base branch from web3/allocations to dev October 4, 2019 00:27
@topocount topocount changed the base branch from dev to web3/allocations October 4, 2019 00:40
Copy link
Member

@ottodevs ottodevs left a comment

Choose a reason for hiding this comment

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

Please @PeterMPhillips I am sorry we didn't warn you about this test-helpers messing that probably confused you, that is one of the reasons we are trying to get rid of it, so it is really important to avoid introducing regressions into our codebase by trying to use it again.

If you are experiencing some kind of issue, please ask us to help with that.

I suggest reverting all these changes, rebasing from latest dev taking care of looking at the Allocations.test.js that has the correct test format and imports, and focus into adding the test cases you need on this file... you should not experience any import error then.

Also avoid modifying Allocations.sol since that code is certified and frozen right now... If we find a critical issue on it, I think it is better tell that in our channels and discuss about the fixes before trying to add the change directly.

apps/allocations/package.json Show resolved Hide resolved
apps/allocations/contracts/Allocations.sol Show resolved Hide resolved
apps/allocations/contracts/Allocations.sol Show resolved Hide resolved
apps/allocations/package.json Show resolved Hide resolved
apps/allocations/test/Spoof.sol Show resolved Hide resolved
apps/allocations/test/allocations.test.js Show resolved Hide resolved
apps/shared/test-helpers/artifacts.js Show resolved Hide resolved
@ottodevs ottodevs added app: allocations testing This issue only requires generation of tests labels Oct 4, 2019
@ottodevs
Copy link
Member

ottodevs commented Oct 4, 2019

I close here in favour of: #1282
Please, move any work from this PR not present on the above, if there is still something relevant here.

Good work overall! I am sorry but things were a bit messed with the different branches.

@ottodevs ottodevs closed this Oct 4, 2019
@chadoh chadoh mentioned this pull request Oct 4, 2019
4 tasks
@ottodevs ottodevs deleted the web3/allocations-improvedtest branch December 15, 2019 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app: allocations testing This issue only requires generation of tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants