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

test: add orchestrator tests #134

Merged
merged 3 commits into from
Feb 17, 2023
Merged

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Feb 14, 2023

Overview

Depends on #133 and #115

Closes #51

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

@rach-id rach-id added orchestrator orchestrator related testing labels Feb 14, 2023
@rach-id rach-id requested a review from rahulghangas February 14, 2023 10:56
@rach-id rach-id self-assigned this Feb 14, 2023
@rach-id rach-id requested a review from evan-forbes as a code owner February 14, 2023 10:56
@rach-id rach-id mentioned this pull request Feb 14, 2023
5 tasks
@codecov-commenter
Copy link

Codecov Report

Merging #134 (696b243) into main (74eb1d9) will increase coverage by 4.93%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #134      +/-   ##
==========================================
+ Coverage   43.27%   48.21%   +4.93%     
==========================================
  Files          15       15              
  Lines         952      952              
==========================================
+ Hits          412      459      +47     
+ Misses        491      438      -53     
- Partials       49       55       +6     
Impacted Files Coverage Δ
orchestrator/orchestrator.go 15.76% <0.00%> (+15.76%) ⬆️
orchestrator/retrier.go 22.22% <0.00%> (+22.22%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

a few minor comments but otherwise LGTM

Comment on lines +25 to +27
func NewOrchestrator(
node *TestNode,
) *orchestrator.Orchestrator {
Copy link
Member

Choose a reason for hiding this comment

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

[non blocking nit]
why format this like this?

Copy link
Member Author

Choose a reason for hiding this comment

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

no reason :D just happened to be like that. should I change?

Copy link
Member Author

Choose a reason for hiding this comment

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

@evan-forbes Can I merge or I should make the change?

Copy link
Member

Choose a reason for hiding this comment

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

up to you! if we don't have linters for it, then I don't think its worth blocking on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, thanks

Comment on lines +35 to +37
if err != nil {
panic(err)
}
Copy link
Member

Choose a reason for hiding this comment

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

[optional]
why not pass a testing.T as an arg to handle errors? are we using this function outside of tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rach-id rach-id merged commit 0d56832 into celestiaorg:main Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
orchestrator orchestrator related testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add unit tests for the copied orchestrator implementation
3 participants