search

SiaFoundation / core

Core packages for the Sia project
MIT License
51 stars 8 forks source link

Add alternative Marshalling Tests for consensus #133

Closed new0nebit closed 11 months ago

new0nebit commented 11 months ago

I finished working on these tests last weekend, but due to unforeseen circumstances, I couldn't submit the pull request. Today I can finally do it, but I noticed that just yesterday another contributor, who coincidentally worked on some of the same test features as I did, submitted his PR #132

I wasn't sure how to proceed in this situation, so I decided to run test coverage. In my case, the overall coverage percentage was 77.3%, while @chris124567's was 79.8%, which I congratulate him on. However, if we combine our PRs, the rate reaches 80.1%

Chris and I approached the task and code-writing differently, and to be honest, I'm not sure how you'll handle our PRs after comparing and analyzing them; nonetheless, I decided to offer alternative tests for the Marshaler and Unmarshaler functions and submit this PR.

lukechampine commented 11 months ago

Thanks! :) I plan to merge a combination of #132, #131, and this PR, with all authors receiving credit.

chris124567 commented 11 months ago

I apologize! I heard someone was contributing to core but I didn't know where. Thank you for covering valid and invalid cases (especially the error cases for unmarshaling Work that I missed). My PR adds tests in a couple other places so the coverage numbers aren't exactly a fair comparison :)

new0nebit commented 11 months ago

No apologies! Don't worry @chris124567 I blame myself for not having the courage to finish the work on time and submit the PR due to silly made-up problems (the rabbit hole of impostor syndrome turned out to be damn deep and exhausting). This will definitely be the most memorable hacktoberfest (ง’̀-‘́)ง Anyway, in the end, I'm glad that we managed to overcome the 80% barrier (･‿･ ) 人 (･‿･ )

lukechampine commented 11 months ago

See #132