Use GOTMPDIR env to clean all the go-build folders

[alessio-perugini]

Proposed changes

Closes #182.

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation Update (if none of the other choices apply)

Checklist

• I have read the CONTRIBUTING doc
• Lint and unit tests pass locally with my changes (make all)
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in downstream modules

Further comments

The proposed PR is an idea to overcome the issue of not cleaning build folders when going into time out.
Writing the test it felt a bit awkward the way I got the root dir of our wdDealer. I think in writing this line:

goTmpDirEnv := fmt.Sprintf("GOTMPDIR=%s", filepath.Dir(mut.Workdir()))

I'm leaking implementation logic in our test.

So I was thinking of another way to get the same result in a cleaner way. We might add a new func in the Dealer interface that returns the root working directory.

type Dealer interface {
	Get(idf string) (string, error)
	WorkDir() string
	Clean()
}

Introduction the WorkDir() getter we can also refactor the implementation in just this 2 lines:

	cmd.Env = append(cmd.Env, os.Environ()...)
	cmd.Env = append(cmd.Env, fmt.Sprintf("GOTMPDIR=" +m.wdDealer.WorkDir()))

WDYT? Should I do the proposed refactoring? Do you suggest other improvements/solutions?

[codecov]

Codecov Report

Merging #183 (e90be61) into main (dab849d) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #183      +/-   ##
==========================================
+ Coverage   88.23%   88.27%   +0.03%     
==========================================
  Files          18       18              
  Lines        1284     1288       +4     
==========================================
+ Hits         1133     1137       +4     
  Misses        126      126              
  Partials       25       25              

Flag	Coverage Δ
unittests	88.27% <100.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
internal/engine/executor.go	91.91% <100.00%> (+0.12%)	⬆️
internal/engine/workdir/workdir.go	75.29% <100.00%> (+0.59%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[k3rn31]

Thanks for the PR. I'd do the refactoring, I don't think we need it in the mutator.

Do you think this also closes #177?

[alessio-perugini]

@k3rn31

• So should we call the os.Setenv("GOTMPDIR", workDir) in the cmd/unleash.go:114 and in the mutator just keep the cmd.Env = append(cmd.Env, os.Environ()...) ?

• Nope unfortunately it doesn't close the Gremlins should use a separate temporary directory as Go build cache #177. I've tried to inject the GOCACHE as I did with the GOTMPDIR but it leads to only TIMEOUT results

[k3rn31]

No, I meant the second approach where the Dealer gives the root. Seems cleaner, so we set it just once, but it is explicit that the temp dir is modified when we call the command.
I prefer that it is explicit we are setting the env variable when we call the command, otherwise it might not be clear and someone can easily introduce bugs there, not knowing.

[k3rn31]

We should add a test to the dealer for WorkDir to be consistent.

[k3rn31]

I added a couple of comments. Please, check if they make sense to you, otherwise we can proceed with the merge.

[k3rn31]

Wouldn't be better to do cmd.Env = os.Environ. Not sure, but I think we can spare an array allocation.

At the beginning Env should be nil, in the first append, it allocates an array with the length of Environ. Then, in the second array it reallocates for the added element, isn't it?
If we assign Environ directly, we are sure at most it reallocates in the only append we have. I'm not certain my reasoning is correct though 😄. What do you thin?

[alessio-perugini]

The only reason I did that is to not break test in: TestMutatorTestExecution. In the executor_test.go:557 we set the GO_TEST_PROCESS=1 if we do not append we will override that breaking 2 tests.

About the allocation, since the os.Environ should be more or less the same every time we call it (at least the env vars we need). We might want to refactor where we assign that line.
We can create our custom execContext func that pre-allocates the env vars, so we will do the allocation and the sys call only once, and not every time we call the runTests func:

func NewExecutorDealer(mod gomodule.GoModule, wdd workdir.Dealer, elapsed time.Duration, opts ...ExecutorDealerOption) *MutantExecutorDealer {
...
	jd := MutantExecutorDealer{
                ...
		execContext:       commandWithEnvs,
	}
...
}
func commandWithEnvs(ctx context.Context, name string, arg ...string) *exec.Cmd {
	cmd := exec.CommandContext(ctx, name, arg...)
	cmd.Env = os.Environ()
	return cmd
}

[k3rn31]

Even if the env is set at every mutator run (potentially a lot of times) I was concerned, but maybe what we gain not reallocating is marginal and doesn't justify this refactoring, what do you think?

[alessio-perugini]

We should perform some benchmarks. I think it won't be the most expensive operation we're doing so we should be fine for now running that code for every mutator.

[k3rn31]

Maybe this test can be integrated in the TestMutatorTestExecution, they are almost identical.