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.

Sign up for GitHub

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

Jump to bottom

Shared: Use time helpers mock in all apps #786

Merged
merged 1 commit into from
Apr 17, 2019
Merged

Shared: Use time helpers mock in all apps #786

merged 1 commit into from
Apr 17, 2019

Conversation

facuspagnuolo
Copy link
Contributor

No description provided.

@facuspagnuolo facuspagnuolo requested a review from sohkai April 12, 2019 14:14
sohkai
sohkai approved these changes Apr 12, 2019
View reviewed changes
Copy link
Contributor

@sohkai sohkai left a comment

Choose a reason for hiding this comment

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

😍

@facuspagnuolo facuspagnuolo changed the base branch from master to update_payroll_w_finance April 12, 2019 17:39
@facuspagnuolo
Copy link
Contributor Author

@sohkai I improved some of the tests to use increasteTime instead of mocking it every time. Feel free to take another look at it.

@facuspagnuolo facuspagnuolo force-pushed the time_helpers_mock branch 2 times, most recently from fd0d26c to 6ba72d4 Compare April 13, 2019 05:10
@facuspagnuolo facuspagnuolo changed the base branch from update_payroll_w_finance to master April 13, 2019 05:26
@aragon aragon deleted a comment from coveralls Apr 13, 2019
@sohkai
Copy link
Contributor

sohkai commented Apr 14, 2019

@facuspagnuolo Looks like the instrumentation during coverage fails due to the new mocks :/. Wonder if we have to add it to the skip list or etc.

@facuspagnuolo
Copy link
Contributor Author

facuspagnuolo commented Apr 15, 2019 •
edited
Loading

@sohkai solidity-coverage is actually failing due to a compilation error that doesn't make sense, it is failing due to the functions being overwritten in the time helpers mock with the following message:

TypeError: Overriding function changes state mutability from "nonpayable" to "view"

@sohkai
Copy link
Contributor

sohkai commented Apr 15, 2019

@facuspagnuolo That's why I think it's due to some instrumentation issue... usually that's the case if we have a view function declared in a contract whose instrumentation gets skipped but another contract that does not get skipped overrides it (since solidity-coverage strips away the view).

Let's not worry about this too much for now and look into it a bit later :).

@facuspagnuolo facuspagnuolo merged commit 92fd5e5 into master Apr 17, 2019
@facuspagnuolo facuspagnuolo deleted the time_helpers_mock branch April 17, 2019 14:24
MickdeGraaf pushed a commit to MickdeGraaf/aragon-apps that referenced this pull request Jan 28, 2020
@facuspagnuolo
Apps: Use shared TimeHelpersMock for tests (aragon#786)
42ca8e3
ramilexe pushed a commit to ConsiderItDone/aragon-apps that referenced this pull request Dec 9, 2021
@facuspagnuolo
Apps: Use shared TimeHelpersMock for tests (aragon#786)
102c0e8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sohkai sohkai sohkai approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@facuspagnuolo @sohkai