intersystems / TestCoverage

Test Coverage Tool
MIT License
9 stars 8 forks source link

Routine Label showing as "Not Covered" when newing variables on same line #34

Closed stevelee12 closed 4 months ago

stevelee12 commented 5 months ago

Before logging this issue, I just wanted to start by thanking @isc-tleavitt for this incredible tool, it's truely awesome!

The issue I've found is when variables are "New'd" on the same line as a routine label, the whole line is marked as "Not Covered" instead of "Covered (x visits)" - see screnshot (tooltip relates to line 230). Is this something that could be easily fixed?

image

Thanks again!

isc-tleavitt commented 5 months ago

This probably wouldn't be too hard to fix. I will have an intern working on TestCoverage enhancements this summer, and this will be a good first task for him.

Relevant code is in one of: TestCoverage.Data.CodeUnitMap:Create TestCoverage.Utils:GetRoutineLineExecutableFlags -> LineIsExecutable -> LineTokenIsExecutable

First need to figure out if the line-by-line monitor shows this line as being executed - if it doesn't, then these methods need to be updated to say that it isn't; if it does, then our mapping to the .MAC routine needs to be fixed for 0 offset.

stevelee12 commented 5 months ago

Wow thanks for the fast response! I noticed as well that routine label lines are not marked as executable, see below (tooltip relates to line 1038). Not sure if this relevant and could be addressed at the same time? image

isc-tleavitt commented 5 months ago

Initially misinterpreted (caffeine still kicking in) - we're bound to what the line-by-line monitor (%SYS.MONLBL / $System.Monitor.LineByLine) can detect as being executed. I don't think it would show line 1038 as being run, but I could be wrong. We'll investigate and fix that too while we're at it if possible

isc-tleavitt commented 4 months ago

@stevelee12 we weren't able to reproduce the error with the line with the tag seemingly not being exercised. What IRIS version are you running on?

We've also confirmed that line 1038 in the second example won't be detected as run in the line-by-line monitor.

stevelee12 commented 4 months ago

Hi Tim

The IRIS version is: containers.intersystems.com/intersystems/irishealth:2022.1.3.670.1

Any particular reason why 1038 isn't detected? Is it because it's a label or is it the ";" character?

Thanks again! :)

isc-tleavitt commented 4 months ago

@stevelee12 there's are no actual ObjectScript commands on the line. In the case of line 230 there is one, so we expect that it could be flagged as executed. Clearly it was, since subsequent lines were!

@isc-cge - note the version listed above. Once you get containers figured out you can circle back to this, but I think #14 is a higher-impact priority at the moment.

stevelee12 commented 4 months ago

Thanks Tim. So in the case of line 1038, a label that gets called by a UnitTest, are you saying that by design you dont expect the label to be marked as coverable. Is that correct? I guess it would be nice if it was coverable because we could see what labels are never touched, but equally, not the end of the world. I'm just trying to understand the scope of what's in/out Thanks

isc-tleavitt commented 4 months ago

@stevelee12 it's not as much a matter of design of TestCoverage as much as the behavior of the underlying IRIS tooling that TestCoverage uses (%SYS.MONLBL - see https://docs.intersystems.com/iris20241/csp/docbook/Doc.View.cls?KEY=GCM_monlbl). Specifically, it seems that %SYS.MONLBL won't flag a line of code as run if it just contains a label and no other ObjectScript commands on it. If we wanted TestCoverage to pick these things up we'd need to do some modification/instrumentation of .INT code, which we've so far managed to avoid. Or we could push for an enhancement to the kernel-level features used in %SYS.MONLBL, which I don't think is really worth it.

Back to the question you'd like answered, for classes, we have the idea of "method coverage" - we haven't gone the same route with routine labels because it's a bit muddier with procedures vs. plain old labels. We could just say that a label is executable if there's ObjectScript between it and the next label, and is hit if ObjectScript between it and the next label is executed. If you'd like to spin this out as a separate issue for tracking purposes, feel free; otherwise, I will.

stevelee12 commented 4 months ago

That sounds perfect! I've logged it as a feature request (#35)

isc-tleavitt commented 4 months ago

Closing this one out as there's no further action for us to take here.