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

Pass setup data object into handleSummary callback #2103

Merged
merged 8 commits into from
Aug 27, 2021
Merged

Pass setup data object into handleSummary callback #2103

merged 8 commits into from
Aug 27, 2021

Conversation

sammieboy97
Copy link
Contributor

Issue: #2088

Passing the setup data into handleSummary callback in the same way how it is in Teardown.

@CLAassistant
Copy link

CLAassistant commented Jul 31, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2021 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@a460e69). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2103   +/-   ##
=========================================
  Coverage          ?   72.41%           
=========================================
  Files             ?      181           
  Lines             ?    14386           
  Branches          ?        0           
=========================================
  Hits              ?    10418           
  Misses            ?     3333           
  Partials          ?      635
Flag Coverage Δ
ubuntu 72.35% <0.00%> (?)
windows 72.04% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a460e69...c309910. Read the comment docs.

imiric
imiric previously requested changes Aug 3, 2021
View reviewed changes
Copy link
Contributor

@imiric imiric left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for this @SamuelJohnson01997!

I think it looks good, but would like to see some tests for it before merging.

P.S.: We're looking into the xk6 test failures, don't worry about it.

@sammieboy97
Copy link
Contributor Author

Sure. On it.

@sammieboy97
Copy link
Contributor Author

Have added the test for the same. Kindly have a look :)

na--
na-- reviewed Aug 24, 2021
View reviewed changes
js/summary.go Outdated Show resolved Hide resolved
@na-- na-- added this to the v0.34.0 milestone Aug 25, 2021
codebien
codebien requested changes Aug 25, 2021
View reviewed changes
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

Hi @SamuelJohnson01997,
thanks for your contribution! I added some tiny suggestions.

js/summary_test.go Outdated Show resolved Hide resolved
js/summary_test.go Show resolved Hide resolved
js/summary_test.go Outdated Show resolved Hide resolved
js/summary.go Outdated Show resolved Hide resolved
sammieboy97 and others added 3 commits August 25, 2021 23:26
@sammieboy97 @codebien
Update js/summary_test.go
7ff0af6 
Checking error before

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>
@sammieboy97 @codebien
Update js/summary_test.go
c589244 
check error after the test.

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>
@sammieboy97
Removed logs in test
e8f0410 
Reverted the newline in import
@sammieboy97
Copy link
Contributor Author

Hi @SamuelJohnson01997,
thanks for your contribution! I added some tiny suggestions.

This is my first Open source contribution. Thanks to you too 😄

@codebien
Copy link
Contributor

Hi @SamuelJohnson01997,
the code looks good to me but the linter is failing. Can you fix it formatting the code, please?

This is my first Open source contribution. Thanks to you too smile

Congrats 🎉

@sammieboy97
Copy link
Contributor Author

Hi @SamuelJohnson01997,
the code looks good to me but the linter is failing. Can you fix it formatting the code, please?

I guess I've fixed it now.

@na-- na-- dismissed imiric’s stale review August 27, 2021 10:39

tests were added

@na-- na-- merged commit 3994f1a into grafana:master Aug 27, 2021
@na-- na-- mentioned this pull request Aug 27, 2021
Closed
@sammieboy97 sammieboy97 deleted the feature/adding-setupdata-in-summary-callback branch August 27, 2021 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@na-- na-- na-- approved these changes

@codebien codebien codebien approved these changes

@imiric imiric Awaiting requested review from imiric

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.34.0
Development

Successfully merging this pull request may close these issues.
6 participants
@sammieboy97 @CLAassistant @codecov-commenter @codebien @imiric @na--