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.

Sign up for GitHub

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

Jump to bottom

bugfix: Fix codecov broken backend coverage upload. #14972

Merged
merged 1 commit into from May 28, 2020
Merged

bugfix: Fix codecov broken backend coverage upload. #14972

merged 1 commit into from May 28, 2020

Conversation

amanagr
Copy link
Member

@amanagr amanagr commented May 14, 2020

Looks like @mateuszmandera 's #13946 introduces this issue. The commits before this have backend coverage. Also, there appears to be some problem with suffix of the file, which should be blank.

Also, removes the find error we see:

find: unknown predicate `-not -path './bower_components/**' -not -path './node_modules/**' -not -path './vendor/**''
 
    Error running `['find', '/home/circleci/zulip', "-not -path './bower_components/**' -not -path './node_modules/**' -not -path './vendor/**'", '-type', 'f', '-name', '*.gcno', '', '-exec', 'gcov', '-pb', '', '{}', '+']`: Command '['find', '/home/circleci/zulip', "-not -path './bower_components/**' -not -path './node_modules/**' -not -path './vendor/**'", '-type', 'f', '-name', '*.gcno', '', '-exec', 'gcov', '-pb', '', '{}', '+']' returned non-zero exit status 1.

@zulipbot
Copy link
Member

Hello @zulip/server-tooling members, this pull request was labeled with the "area: tooling" label, so you may want to check it out!

timabbott
timabbott reviewed May 14, 2020
View reviewed changes
.circleci/config.yml Outdated
@@ -96,7 +96,7 @@ aliases:
name: upload coverage report
command: |
. /srv/zulip-py3-venv/bin/activate
pip install codecov && codecov \
pip install codecov==2.0.15 && codecov \
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

s/undeterministic/nondeterminstic/ (the "un" version isn't a word AFAIK)

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Also can you add more details about the "gov find error"?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

(Like, are newer versions buggy, or is something else going on?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the commit message with what I could find/know about it.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I'd put the issue link in a comment, so it's handy when one's looking at this code.

@amanagr amanagr force-pushed the codecov_broken_backend_upload branch 2 times, most recently from 6ebcfc1 to a538afc Compare May 14, 2020 13:05
@timabbott
Copy link
Sponsor Member

Can you report the data-file option being broken to https://github.com/nedbat/coveragepy/issues/new? If that's indeed what's happening, we should get it fixed upstream; they seem active.

timabbott
timabbott reviewed May 14, 2020
View reviewed changes
tools/coveragerc Outdated
@@ -24,7 +24,6 @@ exclude_lines =
^\s*\.\.\.

[run]
data_file=var/.coverage
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Can you explain this piece of the change a bit more? Will this adjust what path things get written to in a development environment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, .coverage will be written in ~/zulip (or pwd) but .coverage is already in our .gitignore file, so this will not bring any change. Anyways, we are not calling the --no-cov-cleanup argument in development environment, so this file will be deleted.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Right, but I think we want the coverage data to be written to var/.coverage, not the root of the project. Is there a bug in upstream coverage related to this as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not related to coverage but how codecov works when discovering coverage files for uploading. I can't find a setting to specify the location of coverage file when running codecov. (Also google search reveals nothing)

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Can't we just do mv to put things in place for codecov? I think it makes sense to write coveragerc for local development (test-backend --coverage there) and then fix things up for codecov as needed.

mateuszmandera
mateuszmandera reviewed May 14, 2020
View reviewed changes
tools/test-backend
@@ -407,7 +413,6 @@ def main() -> None:
cov.stop()
cov.save()
cov.combine()
cov.data_suffix = False # Disable suffix so that filename is .coverage
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps we should have the data_suffix change as a separate commit? I was confused for a minute about why this was necessary for the no-cov-cleanup option, before realizing that this is a conceptually separate change

Copy link
Member Author

Choose a reason for hiding this comment

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

If it's necessary for this fix, we should keep them in same commit. Let me see their documentation about data_suffix and see if we can do better here.

@amanagr
Copy link
Member Author

amanagr commented May 15, 2020

Filed the issue as nedbat/coveragepy#989

@timabbott
Copy link
Sponsor Member

So, looking at this change, I have a few thoughts:

  • We should submit an issue or support ticket with codecov about their find behavior, and link to that in a comment next to the version-pinning logic.
  • For coverage, maybe we should just pin using an older version that doesn't have the regression where data-suffix doesn't work as intended, rather than changing our configuration for it?

@amanagr amanagr force-pushed the codecov_broken_backend_upload branch from a538afc to 1f7aa2d Compare May 17, 2020 07:55
@amanagr
Copy link
Member Author

amanagr commented May 17, 2020

Updated the commit message with issue links.

For coverage, maybe we should just pin using an older version that doesn't have the regression where data-suffix doesn't work as intended, rather than changing our configuration for it?

It's hard to discover which version introduced this issue, we can wait for someone to reply on the issue are proceeded as they suggest. Although, since we have pinned coverage to 5.1 in our dev.txt, this behaviour will remain the same unless this gets fixed in a future version, which we will possibly know with issue I opened being closed.

@amanagr
Copy link
Member Author

amanagr commented May 17, 2020

Some conversation is going on in nedbat/coveragepy#989, we can wait for them to reply.

@amanagr amanagr force-pushed the codecov_broken_backend_upload branch 3 times, most recently from 6cefa98 to f501c14 Compare May 26, 2020 07:12
@amanagr
ci: Don't remove .coverage file & set empty suffix.
94a8915 
Fixes zulip#14962
* codecov needs `.coverage` file in the pwd to upload coverage
results.
* concurrency='multiprocessing' forces `.coverage` file to have a
data_suffix, we explicity set it to "" for the file to have no suffix.
Filed issue as nedbat/coveragepy#989
@amanagr amanagr force-pushed the codecov_broken_backend_upload branch from f501c14 to 94a8915 Compare May 26, 2020 07:42
@zulipbot zulipbot added size: M and removed size: S labels May 26, 2020
@amanagr
Copy link
Member Author

amanagr commented May 26, 2020

@timabbott IDK how I missed it, but pinning the codecov version to 2.0.15 has error in uploading the files. Wrote a big comment to explain this in the code.

@timabbott timabbott merged commit c9bff71 into zulip:master May 28, 2020
@timabbott
Copy link
Sponsor Member

Look great, merged, thanks @amanagr!

@amanagr amanagr deleted the codecov_broken_backend_upload branch May 28, 2020 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timabbott timabbott timabbott left review comments

@mateuszmandera mateuszmandera mateuszmandera left review comments

Assignees
No one assigned
Labels
area: tooling bug priority: high size: M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@amanagr @zulipbot @timabbott @mateuszmandera