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

Linter fixes for the crypto and encoding k6 modules #3460

Merged
merged 5 commits into from
Dec 7, 2023

Conversation

olegbespalov
Copy link
Contributor

@olegbespalov olegbespalov commented Nov 20, 2023

What?

This PR fixes ~105 linter issues in js/k6/crypto and js/k6/encoding packages.

It's recommended to start looking by commits.

Why?

As we agreed, we aim to fix most of the linter issues.

Part of #769

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make 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)

@olegbespalov olegbespalov added this to the v0.49.0 milestone Nov 20, 2023
@olegbespalov olegbespalov self-assigned this Nov 20, 2023
@olegbespalov olegbespalov changed the title Chore/k6 crypto encoding linter fixes Linter fixes for the crypto and encoding k6 modules Nov 20, 2023
@olegbespalov olegbespalov marked this pull request as draft November 20, 2023 14:58
@olegbespalov
Copy link
Contributor Author

Converted to a draft for data race investigation 🤔

@codecov-commenter
Copy link

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (a087d42) 73.23% compared to head (630412a) 73.11%.

❗ Current head 630412a differs from pull request most recent head 7e0a8ec. Consider uploading reports for the commit 7e0a8ec to get more accurate results

Files Patch % Lines
js/modules/k6/crypto/crypto.go 84.61% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3460      +/-   ##
==========================================
- Coverage   73.23%   73.11%   -0.12%     
==========================================
  Files         267      265       -2     
  Lines       20083    20064      -19     
==========================================
- Hits        14707    14670      -37     
- Misses       4463     4475      +12     
- Partials      913      919       +6     
Flag Coverage Δ
ubuntu 73.11% <84.61%> (-0.05%) ⬇️
windows ?

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

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

@olegbespalov olegbespalov marked this pull request as ready for review November 20, 2023 15:24
@olegbespalov
Copy link
Contributor Author

The latest commit does the trick and fixes the data race. However, I prefer to drop a RandomBytesFailure test case since, in my opinion, it's not a big value to have it 🤷 What do you think?

Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

🚀

I agree with you, the RandomBytesFailure does not look obviously useful to me, and I'd be happy with dropping it indeed 👍🏻

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.

I prefer to drop a RandomBytesFailure test case since, in my opinion, it's not a big value to have it

I don't have a strong opinion but I found it useful, it is testing that in case of an error returned from the reader then the randomBytes function returns an error. Error handling, in the end, is business logic for the randomBytes func.

@olegbespalov olegbespalov merged commit ab237fa into master Dec 7, 2023
23 checks passed
@olegbespalov olegbespalov deleted the chore/k6-crypto-encoding-linter-fixes branch December 7, 2023 09:47
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.

4 participants