benhoyt / goawk

A POSIX-compliant AWK interpreter written in Go, with CSV support
https://benhoyt.com/writings/goawk/
MIT License
1.95k stars 84 forks source link

Code coverage support #154

Closed xonixx closed 2 years ago

xonixx commented 2 years ago

This PR implements #144 and tries to address all CR notes in https://github.com/benhoyt/goawk/issues/144#issuecomment-1223087337.

This PR is based on top of previous related PRs #148, #153 in order to eliminate the necessity for different hacky solutions.

I think code-wise this is ready. The only thing I still want to add - a documentation in own file coverage.md. While I'm working on this I would be really glad if you @benhoyt check the current implementation in this PR. Hopefully now it should be in much better shape than with prevous hacky approach.

benhoyt commented 2 years ago

Thanks! I'll try to take a look in the next few days.

xonixx commented 2 years ago

Documentation is ready as well https://github.com/xonixx/goawk/blob/coverage/cover.md.

benhoyt commented 2 years ago

Hi @xonixx! I've scanned this very briefly, and hope to look at it more closely this week (though it's a busy week ahead).

benhoyt commented 2 years ago

Here's a demo of what I'm referring to with the if and for lines not being green correctly. On the left hand side are Go programs, and on the right are equivalent AWK programs. The top and bottom are two different inputs: an even number, which causes the if "true" block to execute, and an odd number, which causes the "false" block to execute.

We want to AWK coverage to look more like what's on the left.

Screenshot from 2022-10-14 22-58-15

benhoyt commented 2 years ago

Here's another issue with file paths. go tool cover gives an error unless the path starts with / or ./ or ../.

$ cat tst.awk

function add_nums(n) {
    sum = 0
    if (n % 2 == 0) {
        for (i = 0; i < n; i++) {
            sum += i*2
        }
    } else {
        sum = -1
    }
    return sum
}

BEGIN {
    print add_nums(6)
}
$ goawk -coverprofile=cover.out -f tst.awk
30
$ go tool cover -html=cover.out -o cover.html
cover: did not find package for tst.awk in go list output
$ cat cover.out
mode: set
tst.awk:15.2,15.19 1 1
tst.awk:6.4,6.14 1 1
tst.awk:9.3,9.11 1 0
tst.awk:3.2,3.9 1 1
tst.awk:11.2,11.12 1 1

# to get it to work, use "./tst.awk" as the filename:
$ goawk -coverprofile=cover.out -f ./tst.awk
30
$ go tool cover -html=cover.out -o cover.html  # success!
$ cat cover.out
mode: set
./tst.awk:15.2,15.19 1 1
./tst.awk:6.4,6.14 1 1
./tst.awk:9.3,9.11 1 0
./tst.awk:3.2,3.9 1 1
./tst.awk:11.2,11.12 1 1
xonixx commented 2 years ago

Hey @benhoyt! Finally I've addressed all the CR notes.

I've also fixed the bugs you pointed in couple messages above.

The last thing still left to do here is restructuring of command line options in program help string. But this one depends on other PR #155 that needs to be reviewed and merged into this one.

Also, I've noticed that for some reason the Github Actions builds on macos & win is broken. Could you please assist?

xonixx commented 2 years ago

Actually I managed to fix tests for windows. Still there is problem with macOS.

benhoyt commented 2 years ago

Thanks! I'll do the final review later today. It looks like the macOS tests are failing, not because of this PR, but because homebrew gawk has been updated to a newer version (5.2.0), which seems to have something wrong with division? I'm not sure, I'll look into this further later.

benhoyt commented 2 years ago

FYI, I resolve the merge conflicts after merging #155.

benhoyt commented 2 years ago

Yeah, I think we should add a few basic tests for the -d* options -- good call. Feel free to do that in a subsequent PR, or open an issue for me to do it.

benhoyt commented 2 years ago

Actually, I just opened an issue to track that: https://github.com/benhoyt/goawk/issues/156

benhoyt commented 2 years ago

This is great, thank you. Just a couple of nit comments above, and then I'll merge this in. Looking forward to playing with AWK coverage! :-)

xonixx commented 2 years ago

Just a couple of nit comments above, and then I'll merge this in.

Hopefully, done.