Increase test coverage

[everettraven]

Unit tests should be added or modified based on the following analysis of the attached coverage report.

If you would like to contribute, you do not have to increase coverage for all files. Feel free to add a comment to this issue stating which one you would like to pick up and add changes for. When the changes are done, make a pull request with those changes and reference this issue in the pull request comments.

This issue will be closed once coverage on all files are completed.

Coverage Report

To get the coverage report run the following commands from the root directory of the packageless project on git branch main:

go test --coverprofile=coverage.out ./...
go tool cover --html=coverage.out

Analysis of Coverage Report

github.com/everettraven/packageless/main.go does not need modification

github.com/everettraven/packageless/subcommands/install_sc.go

• If possible, tests to test the logic based on different GOOS values should be created.

github.com/everettraven/packageless/subcommands/run_sc.go

• Tests should be added to test that the current working directory is being added to the volumes array if a volume path is equivalent to ""

github.com/everettraven/packageless/subcommands/subcommand.go

• Test should be added to test that the error is being throw properly if the length of the args variable is < 1

github.com/everettraven/packageless/subcommands/uninstall_sc.go

• If possible, tests to test the logic based on different GOOS values should be created.

github.com/everettraven/packageless/subcommands/update_sc.go does not need modification

github.com/everettraven/packageless/subcommands/upgrade_sc.go

• Test should be added to test that the error is being thrown properly if the pim config file does not exist

github.com/everettraven/packageless/subcommands/version_sc.go does not need modification

github.com/everettraven/packageless/utils/alias_unix.go does not need modification

github.com/everettraven/packageless/utils/alias_win.go does not need modification

github.com/everettraven/packageless/utils/docker.go

• Test should be added to cover the case when the length of variable splitVol is != 3

github.com/everettraven/packageless/utils/hcl_parse.go

• Test should be added to cover the case when an unexpected type is passed to the ParseBody function

github.com/everettraven/packageless/utils/mocks.go does not need modification

github.com/everettraven/packageless/utils/utils.go does not need modification

[kastolars]

I would like to give this a shot!

[everettraven]

@kastolars Go for it! Are you interested in taking on the whole issue or an individual file that needs coverage increased?

[kastolars]

@everettraven Hi yes I will review the code and post here which file(s) I think I can cover tomorrow.

[everettraven]

@everettraven Hi yes I will review the code and post here which file(s) I think I can cover tomorrow.

@kastolars Sounds good. Let me know if you have any questions

[kastolars]

I have "Test should be added to test that the error is being throw properly if the length of the args variable is < 1" for /subcommand.go complete

[kastolars]

Next: Test should be added to test that the error is being thrown properly if the pim config file does not exist in upgrade_sc.go

[kastolars]

Next: Tests should be added to test that the current working directory is being added to the volumes array if a volume path is equivalent to "" in run_sc.go

[kastolars]

Haven't started ^ yet, got kind of busy. Will work on it soon as I can.

[everettraven]

Haven't started ^ yet, got kind of busy. Will work on it soon as I can.

@kastolars I merged in your PR, great work on the test changes! Just wanted to mention you are welcome to take as long as you need on any contributions!

[rafiramadhana]

Hi @everettraven.
After the merged PRs from @kastolars , is the Analysis of Coverage Report section above still up to date?

[everettraven]

Hi @everettraven.

After the merged PRs from @kastolars , is the Analysis of Coverage Report section above still up to date?

Hi @neverbeenthisweeb here is an updated one:

Updated Analysis of Coverage Report

github.com/everettraven/packageless/subcommands/install_sc.go

• If possible, tests to test the logic based on different GOOS values should be created.

github.com/everettraven/packageless/subcommands/uninstall_sc.go

• If possible, tests to test the logic based on different GOOS values should be created.

github.com/everettraven/packageless/utils/docker.go

• Test should be added to cover the case when the length of variable splitVol is != 3

github.com/everettraven/packageless/utils/hcl_parse.go

• Test should be added to cover the case when an unexpected type is passed to the ParseBody function

[rafiramadhana]

@everettraven
Got it.
Btw for the docker.go part, I think that should be splitVol instead of volSplit because I can't find any volSplit in the code base (but I can find the splitVol in the RunContainer method).
Wdyt?

[everettraven]

@neverbeenthisweeb Oh, thanks for pointing that out! You are correct, going to fix the typo now.

[everettraven]

Typo fixed in my previous comments

[rafiramadhana]

Got it, thank you @everettraven.

Anyway, I've just read the Docker documentation related to volumes.

It looks like, referring to our code base, Docker only accept either 2 or 3 splitVol:

1. The first one is source
2. The second one is target
3. The third one, which is optional, is an option

In our code base, docker.go#114, it is still possible for user to provide a volume with 1 or >3 splitVols.

Example of 1 splitVol case: /a/path
Example of >3 splitVol case: /a/path:/b/path:abc:def

My question is, should we validate the value volumes passed to RunContainer args to have only either 2 or 3 splitVol? Or it is not the responsibility of RunContainer method? (so perhaps our user should be aware of this case of only volumes of 2 or 3 splitVol that is valid).

This is the error that I have when passing volumes with splitVol value of 1 ([]string{"/another/path"}) of to RunContainer:

panic: runtime error: index out of range [1] with length 1 [recovered]
	panic: runtime error: index out of range [1] with length 1

Maybe we could throw a more user friendly error like: Error: Invalid volumes, should at least have source and target.

Wdyt?

Thank you.

[rafiramadhana]

@everettraven Actually I just realized the case of splitVols value of 1 is still considered as a valid cmd by Docker.

I ran some command similar to this, and Docker still executes this.

$ docker run -d \
--name devtest \
-v myvol2 \
nginx:latest

However, I don't know the mounting behavior of this case (still trying to understand).

[everettraven]

@neverbeenthisweeb
First of all, thanks so much for all the research and thought you have put into this!

I definitely like the idea of throwing a much more friendly error.

I'm not entirely sure if we will ever hit a case where the volume isn't controlled by the logic within packageless since the volumes are dictated by the pim configuration files (can be found at packageless-pims). As of now it only accepts a source and target field and not the extra volume options, which is something I am now thinking should be allowed.

The volumes defined in the pim configuration files is what gets parsed and passed to the RunContainer function when running something like packageless run python

[everettraven]

@neverbeenthisweeb You should be able to see the logic that goes into creating the volume based on the parsed pim configuration file here:

packageless/subcommands/run_sc.go

Line 151 in 449ef84

for _, vol := range version.Volumes {

[rafiramadhana]

@everettraven
I see, sorry I haven't tried packageless app fully, so I don't know about that packageless-pims.

If that's the case, I think we can just assert, whether it will be an error or not for each of splitVols != 3 cases.

Something like:

_, err := util.RunContainer(image, ports, volumes, cName, args)
if err == nil {
    t.Fatalf("RunContainer: Expected to receive an error, but did not receive one.")
} 

Wdyt?

If you're ok, I can create the PR and can continue the discussion there.

Thanks.

[everettraven]

@neverbeenthisweeb No worries on not having tried the app, I'm always happy to help answer any questions about it.

I think that is a good approach so open up a PR whenever you have those changes and I can review it!

[rafiramadhana]

@everettraven
Got it, thanks for the explanation.

Anyway, I've created the PR in #65.

But I forgot to assign you as the reviewer.

Can we add reviewer when the PR is already created here in Github? Sorry I am new to contributing in this platform.

Thank you.

[everettraven]

@neverbeenthisweeb Yes, you should be able to request my review but I can go ahead and add myself on there

[rafiramadhana]

Hi @everettraven

Anyway, I tried to resolve this issue:

github.com/everettraven/packageless/utils/hcl_parse.go

Test should be added to cover the case when an unexpected type is passed to the ParseBody function

in #66.

Please LMK if there is anything I can help with that PR.

Thanks.

Btw, I just read an article in stackoverflow, and it looks like only collaborators that are able to assign reviewers. Maybe this is the reason why I can't assign you as reviewers previously 😁.

[everettraven]

@neverbeenthisweeb Oh gotcha, that makes sense. I will take a look at the PR now!

[everettraven]

Most of the tests in the original issue have been changed to increase coverage so I will be closing this issue. I may open new issues regarding coverage on certain tests in the future.

A huge thank you to both @kastolars and @neverbeenthisweeb for all your help with this issue!