Skip to content

Conversation

ntninja
Copy link
Contributor

@ntninja ntninja commented Aug 24, 2020

The custom registry support is for adding extra MultiAddr codes for specific projects that are either not applicable to general use or, as in my case, blocked on the infinitely slow speed of the MultiAddr spec repo…

Copy link
Contributor

@mhchia mhchia left a comment

Choose a reason for hiding this comment

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

Looks solid! A dynamic protocol registry sounds nice too. I'll have another pass of review since it's relatively big.


[testenv]
commands = pytest --cov=multiaddr --cov-report=term --cov-report=html:build/test-{envname}
commands = pytest --cov=multiaddr --cov-report=term --cov-report=html:build/test-{envname} --cov-fail-under=100
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be necessary to have 100% coverage rate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Define “necessary”. 😉

On that subject, I can only say that for me, it feels a lot better to edit a codebase where it is ensured that at least every line will be run once and most dumb mistakes will be caught. On the other hand, I'm well aware that writing tests isn't most people's favourite activity (myself included) and that it slows down development. My longer term experience has however been that it has generally been worth it however.

Also, note that in this particular case most low-key contributions (like adding a new codec or protocol) will not require writing any test code at all. I fixed the coverage of that previous contribution by simply adding some valid and adverse examples to the relevant test vectors. What I'm saying is, that for most “simple” contributions the overhead for maintaining the coverage should be relatively small. For larger changes (like the ones I tend to contribute), I believe the overhead of writing relevant test is justified for ensuring the longer-term viability of this project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. What I thought unnecessary are something like getters. But yeah they should at least be called once in the tests so 100% coverage should be fine.

@mhchia mhchia mentioned this pull request Sep 4, 2020
Copy link
Contributor

@mhchia mhchia left a comment

Choose a reason for hiding this comment

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

Looks good to me after a second review. Sorry for the delay.


[testenv]
commands = pytest --cov=multiaddr --cov-report=term --cov-report=html:build/test-{envname}
commands = pytest --cov=multiaddr --cov-report=term --cov-report=html:build/test-{envname} --cov-fail-under=100
Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. What I thought unnecessary are something like getters. But yeah they should at least be called once in the tests so 100% coverage should be fine.

@mhchia mhchia merged commit 8ad906c into multiformats:master Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants