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

Missing profiles when merging #172

Closed
gaocegege opened this issue Aug 6, 2020 · 3 comments · Fixed by #177
Closed

Missing profiles when merging #172

gaocegege opened this issue Aug 6, 2020 · 3 comments · Fixed by #177

Comments

@gaocegege
Copy link

gaocegege commented Aug 6, 2020

Hi, I use this branch https://github.com/gaocegege/ormb/tree/goveralls-bug to run goveralls and find that some test coverage is not reported to coveralls.

I find that the logic here has errors: https://github.com/mattn/goveralls/blob/master/gocover.go#L33

I do not think we can assume that each profile has the same sorted FileName and Blocks. I think we need to add more checks for it.

In my case, the test coverage for one profile does not have the same order with others, then it is ignored since we only use the order in head to check it.

Here is a simple fix https://github.com/kleveross/goveralls/pull/1/files:

// mergeProfs merges profiles for same target packages.
// It assumes each profiles have same sorted FileName and Blocks.
func mergeProfs(pfss [][]*cover.Profile) []*cover.Profile {
	// skip empty profiles ([no test files])
	for i := 0; i < len(pfss); i++ {
		if len(pfss[i]) > 0 {
			pfss = pfss[i:]
			break
		}
	}
	if len(pfss) == 0 {
		return nil
	} else if len(pfss) == 1 {
		return pfss[0]
	}

	head, rest := pfss[0], pfss[1:]
	ret := make([]*cover.Profile, 0, len(head))
	for _, profile := range head {
		for _, ps := range rest {
			// find profiles
			if len(ps) == 0 {
				continue
			} else {
				for _, p := range ps {
					if p.FileName == profile.FileName {
						profile.Blocks = mergeProfBlocks(profile.Blocks, p.Blocks)
						break
					}
				}
				continue
			}
		}
		ret = append(ret, profile)
	}
	return ret
}
@mattn
Copy link
Owner

mattn commented Aug 6, 2020

@shogo82148 What do you think?

@gaocegege
Copy link
Author

ping @shogo82148

@shogo82148
Copy link
Collaborator

@gaocegege You are right.
We need to add more checks for it.

It looks that each profile has been sorted by FileName.
https://github.com/golang/tools/blob/39188db5885871ea0132a6314eab0c1b5d179a8b/cover/profile.go#L115

But we shouldn't use ps[i].FileName here, because each profile may not have the same index numbers.

} else if ps[i].FileName != profile.FileName {

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 a pull request may close this issue.

3 participants