Skip to content
This repository has been archived by the owner on Apr 15, 2020. It is now read-only.

Added coverage report #43

Merged
merged 3 commits into from
Aug 8, 2017
Merged

Added coverage report #43

merged 3 commits into from
Aug 8, 2017

Conversation

oed
Copy link
Contributor

@oed oed commented Jul 14, 2017

This PR adds a coverage report using solidity-coverage. It also adds some additional tests.
The coverage report will be availiable on https://uport-project.github.io/uport-identity/coverage once this branch is in master. They are also availiable in the docs/coverage folder

@localredhead localredhead self-assigned this Jul 17, 2017
@oed oed mentioned this pull request Jul 18, 2017
@oed oed force-pushed the feature/148737257-coverage-report branch from f3f7ebd to 091c0ec Compare August 2, 2017 16:12
@oed
Copy link
Contributor Author

oed commented Aug 2, 2017

Some things I found that might be worth updating in the (Meta)IdentityManager contracts while updating the tests:

  • migrationInitiated & migrationNewAddress should maybe be public?
  • Check in finalizeMigration should throw, otherwise there is no way of knowing if the call failed
  • Add isOlderOwner constant function?

@localredhead localredhead self-requested a review August 3, 2017 08:16
Copy link
Contributor

@localredhead localredhead left a comment

Choose a reason for hiding this comment

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

LGTM - 1 more review please

@@ -0,0 +1,158 @@
var addSorting = (function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm. Ok, just to clarify and make sure I understand correctly, the .html files above are essentially ways of displaying the coverage of the contracts. Is this file for helping with that task also?

@@ -0,0 +1,158 @@
var addSorting = (function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm - is this a file repeat? Not sure I totally follow what is happening here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all files in the coverage folder are auto generated :)

Copy link
Contributor

@naterush naterush left a comment

Choose a reason for hiding this comment

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

The changes to the tests look great. I'm assuming the rest of the changes are the coverage report, which I guess is good too, though I don't know much :)

@oed oed merged commit f90f9ed into develop Aug 8, 2017
@naterush naterush deleted the feature/148737257-coverage-report branch November 15, 2017 22:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants