Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

increase coverage on integration tests #120

Merged
merged 9 commits into from
May 7, 2021
Merged

Conversation

J-Thompson12
Copy link
Contributor

@J-Thompson12 J-Thompson12 commented May 6, 2021

I looked it over and this looks like it is accurate on its coverage.

@J-Thompson12 J-Thompson12 marked this pull request as ready for review May 6, 2021 20:41
@luizbafilho
Copy link
Contributor

luizbafilho commented May 6, 2021

Looks like its trying to get coverage from the fakes: https://coveralls.io/builds/39429875/source?filename=pkg%2Ffluxops%2Ffluxopsfakes%2Ffake_flux_handler.go

I've seen there is a ignore option in go-acc has a --ignore flag, could you try calling it with fakes?

@J-Thompson12
Copy link
Contributor Author

@luizbafilho go-acc can use any flags from go test. I have it set to test every go file in the dir. I should add something to skip those though. Good catch.

Copy link
Contributor

@jrryjcksn jrryjcksn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\o/

)

func TestFileExists(t *testing.T) {
require.True(t, FileExists("utils.go"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kind suggestion. It would be great to have a file used specifically for this test. In case the rare case utils.go changes name. There are some methods to create temp files like this:

tempFile, err := ioutil.TempFile("", "")
os.Remove(tempFile.Name())

Then we don't need to rely any external resources to test this function 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is a very valid point

Copy link
Contributor

@josecordaz josecordaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@J-Thompson12 J-Thompson12 merged commit 1e93936 into main May 7, 2021
@J-Thompson12 J-Thompson12 deleted the integration-coverage branch May 27, 2021 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants