Fix position parser for base pointer options#5574
Fix position parser for base pointer options#5574chrismooreproductions wants to merge 1 commit intografana:masterfrom
Conversation
chrismooreproductions
requested review from
AgnesToulet and
oleiade
and removed request for
a team
|
|
examples/browser/tap-position.js
Outdated
| } finally { | ||
| await page.close() | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think this example is technically required, but it looks like we don't have explicit tests for the element handle options, and this highlights the working functionality.
There was a problem hiding this comment.
I would prefer an integration test, or even just a unit test. Since the issue was in the Parse logic maybe we should have a unit test on that, and then an integration test in the tests package to ensure that we don't regress in the future.
There was a problem hiding this comment.
No problem. I may look to expand this PR to cover a few other cases as discussed in #4413, and I had a thought that I can introduce some unit tests along the way.
Leave it with me (and thanks for the review 🫶)
There was a problem hiding this comment.
Hi @ankur22 , hope you are well!
Sorry for the delay, I have pushed the suggested changes and deleted this file.
If you could review when you get that chance that would be fab.
Thanks!
Chris
chrismooreproductions
changed the title
|
@Ankur I'll dismiss myself in your favor as you opened that issue if that's okay with you 🙇🏻 |
| var p map[string]float64 | ||
| o.Position = &Position{} | ||
| if rt.ExportTo(opts.Get(k), &p) != nil { | ||
| if rt.ExportTo(opts.Get(k), &p) == nil { |
There was a problem hiding this comment.
I like easy fixes 😄 Thanks!
There was a problem hiding this comment.
Ha!
I went down a bit of a rabbit hole with the sobek types but the fix was simple in the end :)
There was a problem hiding this comment.
@ankur22 Shouldn't we follow the same pattern as in the other functions and return the error if it's not nil?
Something like (not applicable directly as I moved line 190 after the error check:
| if rt.ExportTo(opts.Get(k), &p) == nil { | |
| if err := rt.ExportTo(opts.Get(k), &p); err != nil { | |
| return fmt.Errorf("parsing element handle base pointer option position: %w", err) | |
| } | |
| o.Position = &Position{} | |
| o.Position.X = p["x"] | |
| o.Position.Y = p["y"] |
There was a problem hiding this comment.
Yes correct! Good spot! @chrismooreproductions please take a look at what @AgnesToulet has pointed out. You should be able to find other examples in the codebase where we are doing this, as well as the suggestion itself.
There was a problem hiding this comment.
N/P thanks Agnes! I will fix this up and get back to you shortly
There was a problem hiding this comment.
Hey guys sorry for the delay on this, I have pushed the suggested change. Nice catch Agnes :)
ankur22
left a comment
ankur22
left a comment
There was a problem hiding this comment.
Thanks for taking a look and fixing the issue!
I think it's worth adding a couple of tests to encapsulate this behaviour and avoid regressions.
chrismooreproductions
force-pushed
the
fix/position-base-pointer-options-parser
branch
from
fe18489 to
0d24d9e
Compare
Fixes an incorrect error check in parsing logic Closes grafana#4429
chrismooreproductions
force-pushed
the
fix/position-base-pointer-options-parser
branch
from
0d24d9e to
286fe2b
Compare
5 participants
What?
Fixes an incorrect error check in the parser code for the
positionoption, with an example of the correct functionality.Why?
As in #4429, the
positionoptions weren't being correctly parsed meaningtap()always tapped at position (0,0).Checklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
Closes #4429