k6/html: lint fixes #3701
k6/html: lint fixes #3701
Conversation
mstoykov
requested review from
oleiade and
joanlopez
and removed request for
a team
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3701 +/- ##
==========================================
+ Coverage 73.28% 73.29% +0.01%
==========================================
Files 278 278
Lines 20389 20389
==========================================
+ Hits 14942 14945 +3
+ Misses 4497 4494 -3
Partials 950 950
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
mstoykov
changed the title
js/modules/k6/html/element_test.go
Outdated
| @@ -210,8 +210,8 @@ func TestElement(t *testing.T) { | |||
|
|
|||
| assert.True(t, ok) | |||
| assert.Equal(t, 9, len(nodes)) | |||
| assert.Contains(t, nodes[0].Export().(Element).TextContent(), "innerfirst") | |||
| assert.Contains(t, nodes[8].Export().(Element).TextContent(), "innerlast") | |||
| assert.Contains(t, nodes[0].Export().(Element).TextContent(), "innerfirst") //nolint:forcetypeassert | |||
There was a problem hiding this comment.
Could make sense to disable forcetypeassert linter for all the test files? While I completely see the value of that linter for production Go code, I don't fully see it for tests (as in the worst case, the test would fail?).
I've seen many annotations (+20) like this and most of them are in test files, and I started to feel like ol' good times in Java with annotations everywhere 😅
There was a problem hiding this comment.
I am not really against this, and it might be easier to do it 🤷
cc @grafana/k6-core anyone against it?
There was a problem hiding this comment.
I agree with Joan, in the context of tests, I always assume the person writing these kind of checks know what they're doing and why, so 👍🏻 on my side 🙇🏻
mstoykov
force-pushed
the
fixHTMLModuleLintIssues
branch
from
9c637c5
to
4fa1169
Compare
mstoykov
force-pushed
the
dropForceTypeAssertForTests
branch
from
d5be7db
to
64e4aac
Compare
mstoykov
dismissed stale reviews from oleiade and joanlopez
The base branch was changed.
While nice to have it it mostly gives lint errors for not documenting every export method. As we haven't fixed those in 6 years I don't think we will ever do it for the 100+ methods. As such it seems better to just ignore it and lower the golangci-lint noise. Unfortunately using "//revive:disable:exported" in the file itself did not work with golangci-lint.
mstoykov
force-pushed
the
fixHTMLModuleLintIssues
branch
from
653a1ba
to
18d4709
Compare
What?
Fix lint issues inthe whole "k6/html" ... part of it through ignoring lint issues :)
Why?
Part of #769.
Separate commits explained additional reasoning for each change
Checklist
make lint) and all checks pass.make tests) and all tests pass.Related PR(s)/Issue(s)