Solving missing line param on gitlab codequality #298
Conversation
|
This PR currently undoes some of my requested changes on #294. Please go back through those and ensure they are all done. In addition, I can tell just by looking that the given test will fail. Please run the tests and fix the failure. |
|
Hello, @braydonk. |
|
I still don't see evidence that the author of this PR actually ran the tests (I know they will fail by reading). In addition, old comments I made on the PR were undone in this reopen. |
|
Hi @braydonk , the local test output: marciovieira@MacBook-Pro-de-Marcio yamlfmt % make test |
|
Regarding the comments you mentioned in the other PR, I believe I've resolved them; I'm not understanding what the points in question are. |
The particular comment that was undone was referring the
Thanks for the test output. I'm sorry for sounding accusatory, I have to be cautious with the influx of AI Slop PRs. The problem is that your local branch is not properly based on latest main. I approved the GitHub Actions on your latest commit so you can see the failure related to the code error I saw. |
|
@braydonk can you review again please? |
|
2 things:
|
I used |
d128552 to
32ee3ef
Compare
|
@braydonk Rebased |
|
@braydonk can you review again? Still Waiting ur answer |
braydonk
left a comment
There was a problem hiding this comment.
Is there any way to validate that this new codequality result will be properly loaded by Gitlab? I don't use Gitlab so I don't know and have no way of verifying.
internal/gitlab/codequality_test.go
Outdated
| t.Parallel() | ||
|
|
||
| testdataDir := "../../testdata/gitlab/changed_line" | ||
| print(testdataDir) |
internal/gitlab/codequality_test.go
Outdated
| func TestCodeQuality_DetectChangedLine(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| testdataDir := "../../testdata/gitlab/changed_line" |
There was a problem hiding this comment.
There should be a new testdata directory in this package, rather than a new folder in the testdata package at the root of the project. So the path should just be ./testdata/changed_line from here.
| begin = i + 1 | ||
| } | ||
| lineNum := i + 1 | ||
| end = &lineNum |
There was a problem hiding this comment.
This will generate a lot of unnecessary heap garbage. This function should just return an int instead of *int so you can avoid that complexity.
internal/gitlab/codequality.go
Outdated
| original := strings.Split(string(diff.Diff.Original), "\n") | ||
| formatted := strings.Split(string(diff.Diff.Formatted), "\n") | ||
|
|
||
| max := len(original) |
There was a problem hiding this comment.
This can just use math.Max instead.
There was a problem hiding this comment.
math.Max is for float64, so you'll be entering int to float64 to int instructions without clearly gaining anything. I can't say if this is the best way.
There was a problem hiding this comment.
Right, perhaps we can use the max builtin` then.
internal/gitlab/codequality_test.go
Outdated
| formattedPath := filepath.Join(testdataDir, "formatted.yaml") | ||
|
|
||
| original, err := os.ReadFile(originalPath) | ||
| if err != nil { |
There was a problem hiding this comment.
All these tests should use the internal/assert package to assert their results instead of this if -> t.Fatal pattern.
| } | ||
| } | ||
|
|
||
| func TestCodeQuality_DetectChangedLine(t *testing.T) { |
There was a problem hiding this comment.
This should be a table test with more test cases. The testdata directory should have multiple different examples of changed lines. Some edge cases I would like to see:
- No lines changed
- All lines changed
- One line changed
- Only final line changed
- Only change is one final line added
A compliant report need to output lines on "location" object to work properly as doc in link show us: https://docs.gitlab.com/ci/testing/code_quality/
This commit write de "lines" property properly. If "location" object don't output the line, the gitlab show a message saying "Failed to load Code Quality report"
Issue: #272