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

Improve test coverage for encode.ts file #47

Merged
merged 12 commits into from
Oct 8, 2024

Conversation

lukasbicus
Copy link
Contributor

@lukasbicus lukasbicus commented Oct 2, 2024

Current improvement:
image

Hi @xseman. Please check the implementation. The main change is, that I have introduced the EncodeError class with the EncodeErrorMessage enum. What do you think about that approach?

@lukasbicus lukasbicus marked this pull request as draft October 2, 2024 14:33
@xseman
Copy link
Owner

xseman commented Oct 4, 2024

If you prefer using EncodeError instead of a plain Error, that's fine with me. The main goal for increasing coverage is to test the untested branching logic.

I'm aiming for about 90% branch coverage per file, but any improvement is great! It might not be possible to increase it further because reports are still experimental. If you think we've hit the limit, I'm happy with the PR as is.

@lukasbicus
Copy link
Contributor Author

@xseman - I asked because this can lead to a major breaking change. On the other hand, I see an added value in creating an enum of errors and moving the changing parts of error response to extensions. It's much easier to process that structure on a frontend. I will continue with the encode.ts file. I believe I will reach 100% coverage or near.

@lukasbicus lukasbicus marked this pull request as ready for review October 7, 2024 12:35
@lukasbicus
Copy link
Contributor Author

image

@xseman
Copy link
Owner

xseman commented Oct 7, 2024

Nice, it looks like you've handled everything with the encoding. If you're not working on decoding, we can merge it.

@lukasbicus
Copy link
Contributor Author

I have assigned some time for bysquere on Friday. So this can be merged and I will open another PR or I can continue within this one on Friday.

@xseman xseman merged commit e7cc405 into xseman:master Oct 8, 2024
2 checks passed
xseman pushed a commit that referenced this pull request Oct 8, 2024
* Add test for headerBysquare
* Add test for headerBysquare version
* Add test for headerBysquare document type and reserved nibble
* Add test for direct debit.
* Add test for removeDiacritics
@xseman xseman mentioned this pull request Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants