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

feat: enabling the extraction of a specific selection from an element #3519

Merged

Conversation

Azhovan
Copy link
Contributor

@Azhovan Azhovan commented Dec 28, 2023

What?

This pull request introduces enhancements to the k6 HTML module, including the addition of the Selection function. Its is designed to improve HTML parsing and manipulation capabilities within k6:

  • Selection Method: Added to the Element type, it facilitates operations on the HTML content represented within an Element.

issue: #2683

Why?

  • Addresses the need for a more intuitive API for DOM element selection and manipulation in performance testing scripts.

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)

@CLAassistant
Copy link

CLAassistant commented Dec 28, 2023

CLA assistant check
All committers have signed the CLA.

@Azhovan Azhovan force-pushed the jabar/extracting-selections-from-elements branch 2 times, most recently from b715a27 to b36d3ec Compare December 28, 2023 22:33
@codecov-commenter
Copy link

codecov-commenter commented Dec 29, 2023

Codecov Report

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

Comparison is base (7d131d6) 72.48% compared to head (6203dce) 72.47%.
Report is 13 commits behind head on master.

❗ Current head 6203dce differs from pull request most recent head 139495f. Consider uploading reports for the commit 139495f to get more accurate results

Files Patch % Lines
cmd/builtin_output_gen.go 16.66% 19 Missing and 1 partial ⚠️
cmd/report.go 63.41% 15 Missing ⚠️
execution/scheduler.go 57.57% 7 Missing and 7 partials ⚠️
js/modules/resolution.go 0.00% 5 Missing ⚠️
cmd/outputs.go 62.50% 2 Missing and 1 partial ⚠️
execution/controller.go 75.00% 1 Missing and 1 partial ⚠️
cmd/inspect.go 0.00% 1 Missing ⚠️
cmd/run.go 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3519      +/-   ##
==========================================
- Coverage   72.48%   72.47%   -0.01%     
==========================================
  Files         276      278       +2     
  Lines       20842    20933      +91     
==========================================
+ Hits        15108    15172      +64     
- Misses       4771     4786      +15     
- Partials      963      975      +12     
Flag Coverage Δ
ubuntu 72.47% <56.42%> (+0.03%) ⬆️
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.

@Azhovan Azhovan force-pushed the jabar/extracting-selections-from-elements branch from b36d3ec to 50d21c8 Compare December 29, 2023 11:28
@Azhovan Azhovan marked this pull request as ready for review December 29, 2023 11:28
@Azhovan Azhovan requested a review from a team as a code owner December 29, 2023 11:28
@Azhovan Azhovan requested review from codebien and joanlopez and removed request for a team December 29, 2023 11:28
@joanlopez joanlopez added the documentation-needed A PR which will need a separate PR for documentation label Jan 2, 2024
@joanlopez
Copy link
Contributor

It looks generally good, despite of the minor suggestions I left. However, after reading the discussion on the issue, I am not super confident about whether or not we want to expose/add (all) these methods. So, adding Mihail as reviewer, cause I've seen he's been involved in the discussion and so might have some feedback/thoughts to share around that.

Thanks!

@joanlopez joanlopez requested a review from mstoykov January 2, 2024 10:54
@Azhovan
Copy link
Contributor Author

Azhovan commented Jan 3, 2024

what do you think @mstoykov about @joanlopez comment above

I am not super confident about whether or not we want to expose/add (all) these methods

@mstoykov
Copy link
Contributor

mstoykov commented Jan 10, 2024

I think Locator was just something @w1kman talked about, but shouldn't really be part of this fix at all.

The FromElement was based on the idea that Selection is the prototype that is implemented by each selection, and it will have a method (like static/moduled method in most other cases) and that will be used.

As we do not have currently a Selection prototype (AFAIK) - this isn't really possible.

As such I feel like we should just go with the selection method and drop everything else?

p.s. sorry I was away for the holidays and this then fell through the cracks 🙇

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.

LGTM!

I kind of would prefer if the test was with teh original example, but not a big problem as long as it works.

@mstoykov mstoykov added this to the v0.49.0 milestone Jan 11, 2024
@mstoykov mstoykov requested a review from joanlopez January 11, 2024 14:27
@mstoykov mstoykov merged commit 04a8119 into grafana:master Jan 11, 2024
22 checks passed
mstoykov added a commit that referenced this pull request Jan 16, 2024
mstoykov added a commit to grafana/k6-docs that referenced this pull request Jan 16, 2024
mstoykov added a commit to grafana/k6-docs that referenced this pull request Jan 17, 2024
mstoykov added a commit to grafana/k6-docs that referenced this pull request Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-needed A PR which will need a separate PR for documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants