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

Add documentation to k6 exit codes definitions #3200

Merged
merged 3 commits into from
Sep 4, 2023

Conversation

oleiade
Copy link
Member

@oleiade oleiade commented Jul 17, 2023

What?

This Pull Request adds Godoc documentation comment to k6 exit codes.

I've added my context, but if reviewers have more about specific exit codes they wish to add, that would be the perfect opportunity.

Why?

Having recently been in the situation of interpreting the status code (on-call) for debugging purposes, I found that in some instances, the reason and context for each status code could have been more straightforward. Hence, I added documentation to each of them, albeit probably not complete and not having as much context as possible.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make ci-like-lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

@oleiade oleiade self-assigned this Jul 17, 2023
@github-actions github-actions bot requested review from imiric and mstoykov July 17, 2023 08:44
Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

Great stuff ❤️ Thanks for doing this 🙇

To be more precise, one possible improvement could be to explicitly name k6's REST API in the places where it's mentioned. What do you think?

@oleiade
Copy link
Member Author

oleiade commented Jul 17, 2023

I'm sorry, I'm not sure to understand what you're suggesting. What changes do you have in mind? 🙇🏻

olegbespalov
olegbespalov previously approved these changes Jul 17, 2023
Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

@oleiade I've added direct suggestion feel free to accept or drop them

errext/exitcodes/codes.go Outdated Show resolved Hide resolved
errext/exitcodes/codes.go Outdated Show resolved Hide resolved
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

Save for a couple of cases like ExternalAbort, most of these comments seem redundant to me, as they repeat in prose what is already self-explained by the constant names. But if you and others find this helpful, I'm not against it.

@olegbespalov
Copy link
Contributor

@imiric they might seem redundant, but in fact, such comments also help go documentation properly parse and put such constants into the proper place. E.g. right now the section is empty https://pkg.go.dev/go.k6.io/[email protected]/errext/exitcodes#pkg-constants

For sure the consts themself, are still presented and self-explanatory, but they also provide a better UX with a bit of cost.

@oleiade
Copy link
Member Author

oleiade commented Aug 7, 2023

I agree that they might come up as redundant, but they're actually the result of me struggling to match k6 exit codes to their meaning "easily".

I'd still like to merge them, and eventually even improve them 👍

@mstoykov mstoykov added this to the v0.47.0 milestone Aug 16, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2023

Codecov Report

Patch coverage: 67.00% and project coverage change: +0.27% 🎉

Comparison is base (5f5e374) 72.85% compared to head (11215c6) 73.13%.
Report is 66 commits behind head on master.

❗ Current head 11215c6 differs from pull request most recent head 8bab212. Consider uploading reports for the commit 8bab212 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3200      +/-   ##
==========================================
+ Coverage   72.85%   73.13%   +0.27%     
==========================================
  Files         256      256              
  Lines       19804    19890      +86     
==========================================
+ Hits        14429    14546     +117     
+ Misses       4474     4414      -60     
- Partials      901      930      +29     
Flag Coverage Δ
ubuntu 73.13% <67.00%> (+0.33%) ⬆️
windows ?

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

Files Changed Coverage Δ
api/v1/setup_teardown_routes.go 55.55% <0.00%> (-3.27%) ⬇️
js/jsmodules.go 100.00% <ø> (ø)
js/modules/k6/experimental/tracing/propagator.go 100.00% <ø> (ø)
lib/consts/consts.go 78.57% <ø> (ø)
lib/executors.go 93.10% <ø> (ø)
metrics/engine/ingester.go 91.48% <ø> (ø)
output/cloud/expv2/collect.go 92.59% <ø> (+0.51%) ⬆️
output/cloud/v1/output.go 57.45% <0.00%> (-10.36%) ⬇️
cloudapi/insights/client.go 62.20% <12.50%> (-17.18%) ⬇️
js/modules/k6/experimental/tracing/module.go 69.09% <50.00%> (-2.61%) ⬇️
... and 17 more

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

I have the same opinion as @imiric, but I guess if others find it useful -let's do it.

@oleiade oleiade merged commit bec53bf into master Sep 4, 2023
12 of 20 checks passed
@oleiade oleiade deleted the improve-exit-codes-documentation branch September 4, 2023 13:19
oleiade added a commit that referenced this pull request Sep 8, 2023
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.

5 participants