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

feat(headertest): Validate exact error types in header tests #2644

Merged
merged 11 commits into from
Sep 7, 2023
Merged

feat(headertest): Validate exact error types in header tests #2644

merged 11 commits into from
Sep 7, 2023
+20 −14

Conversation

Manav-Aggarwal
Copy link
Member

@Manav-Aggarwal Manav-Aggarwal commented Aug 31, 2023 •
edited
Loading

Overview

Closes: #2645

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@github-actions github-actions bot added the external Issues created by non node team members label Aug 31, 2023
@Manav-Aggarwal Manav-Aggarwal self-assigned this Aug 31, 2023
@Manav-Aggarwal Manav-Aggarwal added kind:testing Related to unit tests errors All issues related to error handling labels Aug 31, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2023 •
edited
Loading

Codecov Report

Merging #2644 (6cacfc6) into main (c146a0a) will increase coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2644      +/-   ##
==========================================
+ Coverage   51.00%   51.14%   +0.14%     
==========================================
  Files         158      158              
  Lines       10657    10657              
==========================================
+ Hits         5436     5451      +15     
+ Misses       4744     4731      -13     
+ Partials      477      475       -2

see 5 files with indirect coverage changes

@Manav-Aggarwal Manav-Aggarwal changed the title Validate exact error types in header tests feat(headertest): Validate exact error types in header tests Aug 31, 2023
@Manav-Aggarwal Manav-Aggarwal marked this pull request as ready for review August 31, 2023 18:27
distractedm1nd
distractedm1nd previously approved these changes Sep 1, 2023
View reviewed changes
Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

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

Thank you for this <3

renaynay
renaynay requested changes Sep 4, 2023
View reviewed changes
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

I don't agree with this approach - we'd have nested error types with nested reasons. I don't think this is necessary nor good UX.

header/headertest/verify_test.go Outdated Show resolved Hide resolved
Wondertan
Wondertan requested changes Sep 4, 2023
View reviewed changes
header/header_errors.go Outdated Show resolved Hide resolved
@Manav-Aggarwal Manav-Aggarwal force-pushed the manav/validate_exact_errors_in_header_verify_test branch from 05de04c to 08a9e9a Compare September 4, 2023 23:11
Wondertan
Wondertan requested changes Sep 5, 2023
View reviewed changes
header/header.go Outdated Show resolved Hide resolved
header/headertest/verify_test.go Outdated Show resolved Hide resolved
@Manav-Aggarwal Manav-Aggarwal force-pushed the manav/validate_exact_errors_in_header_verify_test branch 2 times, most recently from 20a519f to e12581e Compare September 5, 2023 18:16
Wondertan
Wondertan requested changes Sep 5, 2023
View reviewed changes
header/header.go Outdated Show resolved Hide resolved
header/headertest/verify_test.go Outdated Show resolved Hide resolved
@Manav-Aggarwal Manav-Aggarwal force-pushed the manav/validate_exact_errors_in_header_verify_test branch from 32c23b3 to 6cacfc6 Compare September 5, 2023 23:17
Wondertan
Wondertan reviewed Sep 6, 2023
View reviewed changes
header/header.go Outdated Show resolved Hide resolved
header/header.go Outdated Show resolved Hide resolved
Wondertan
Wondertan approved these changes Sep 6, 2023
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

It's a much neater diff now! Thanks

@Manav-Aggarwal Manav-Aggarwal force-pushed the manav/validate_exact_errors_in_header_verify_test branch from 2de5688 to 2b57bac Compare September 6, 2023 15:22
@renaynay renaynay merged commit a3d4cd5 into celestiaorg:main Sep 7, 2023
13 of 15 checks passed
renaynay pushed a commit to renaynay/celestia-node that referenced this pull request Sep 13, 2023
@Manav-Aggarwal @Wondertan @renaynay
feat(headertest): Validate exact error types in header tests (celesti…
8315c18 
…aorg#2644)

<!--
Please read and fill out this form before submitting your PR.

Please make sure you have reviewed our contributors guide before
submitting your
first PR.
-->

## Overview
Closes: celestiaorg#2645 
<!-- 
Please provide an explanation of the PR, including the appropriate
context,
background, goal, and rationale. If there is an issue with this
information,
please provide a tl;dr and link the issue. 
-->

## Checklist

<!-- 
Please complete the checklist to ensure that the PR is ready to be
reviewed.

IMPORTANT:
PRs should be left in Draft until the below checklist is completed.
-->

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [ ] Required CI checks are passing
- [ ] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords

---------

Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
@faddat faddat mentioned this pull request Feb 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wondertan Wondertan Wondertan approved these changes

@renaynay renaynay renaynay approved these changes

@vgonkivs vgonkivs Awaiting requested review from vgonkivs vgonkivs is a code owner

@walldiss walldiss Awaiting requested review from walldiss walldiss is a code owner

@distractedm1nd distractedm1nd Awaiting requested review from distractedm1nd

Assignees

@Manav-Aggarwal Manav-Aggarwal

Labels
errors All issues related to error handling external Issues created by non node team members kind:testing Related to unit tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validate exact expected errors in header verify tests
5 participants
@Manav-Aggarwal @codecov-commenter @distractedm1nd @Wondertan @renaynay