Skip to content

HelpersTask487_Add_Git_actions_for_Codecov_3 #516

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

Closed
wants to merge 18 commits into from

Conversation

Shaunak01
Copy link
Contributor

@Shaunak01 Shaunak01 commented Apr 8, 2025

@Shaunak01 Shaunak01 self-assigned this Apr 8, 2025
@Shaunak01 Shaunak01 force-pushed the HelpersTask487_Add_Git_actions_for_Codecov_3 branch from 174b5f5 to 90a7090 Compare April 9, 2025 03:47
@Shaunak01 Shaunak01 mentioned this pull request Apr 9, 2025
3 tasks
@Shaunak01 Shaunak01 force-pushed the HelpersTask487_Add_Git_actions_for_Codecov_3 branch from 15de243 to 6693bd5 Compare April 9, 2025 21:00
Copy link

codecov bot commented Apr 9, 2025

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@Shaunak01
Copy link
Contributor Author

@gpsaggese @dremdem

Take a look at how the coverage looks: https://app.codecov.io/github/causify-ai/helpers/tree/HelpersTask487_Add_Git_actions_for_Codecov_3/

We get a 14 day free trial for complete org so we can do that if we like this service.

@Shaunak01 Shaunak01 requested review from gpsaggese and dremdem April 9, 2025 21:28
not hgit.is_in_helpers_as_supermodule(),
reason="""Skip unless helpers is the supermodule. Fails when updating submodules;
passes in fast tests super-repo run. See CmTask10845.""",
not hgit.is_in_helpers_as_supermodule() or os.getenv("CSFY_CI") == "true",
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a is_ci function somewhere in hserver

@samarth9008
Copy link
Contributor

@dremdem

Let me know if you have any blockages.

@Shaunak01

The working should be similar to the current coverage run

Copy link
Contributor

@dremdem dremdem left a comment

Choose a reason for hiding this comment

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

For draft PR is looking good.
Small nits.
Considering
https://github.com/causify-ai/helpers/pull/516/files#r2036238705
Need to rework it as separate GH actions workflows for fast, slow and superslow.

@Shaunak01 Shaunak01 force-pushed the HelpersTask487_Add_Git_actions_for_Codecov_3 branch from f10095d to 8a51bda Compare April 12, 2025 00:24
@Shaunak01
Copy link
Contributor Author

@dremdem

I am trying to generate separate and combined coverage reports for fast and slow tests to see sep reports on copdecov. Initially, the default approach worked by using the implicit normalization of file paths in the default .coverage file, but in my approach we renamed the files and explicitly set the data file with the COVERAGE_FILE variable, absolute paths (like /app/config_root/__init__.py and /app/conftest.py) began causing errors because Coverage couldn't find the corresponding local source. Now, I am exploring ways to either map these paths, ignore errors, or otherwise replicate the default behavior so that every file is included in the report without manual omission.

@Shaunak01 Shaunak01 force-pushed the HelpersTask487_Add_Git_actions_for_Codecov_3 branch from 924b3d6 to 7e60ad5 Compare April 12, 2025 02:53
@Shaunak01
Copy link
Contributor Author

Shaunak01 commented Apr 14, 2025

@gpsaggese @dremdem

Pls take a look at: https://app.codecov.io/github/causify-ai/helpers

We can have the coverage for just the fast tests, slow tests or you can select both flags to see combined

FYI the combined and helpers flag are useless so ignore

@dremdem
Copy link
Contributor

dremdem commented Apr 15, 2025

@gpsaggese @dremdem

Pls take a look at: https://app.codecov.io/github/causify-ai/helpers

We can have the coverage for just the fast tests, slow tests or you can select both flags to see combined

FYI the combined and helpers flag are useless so ignore

It looks good.
Probably later we can also integrate it into PRs statuses:
https://github.com/marketplace/codecov

@dremdem
Copy link
Contributor

dremdem commented Apr 15, 2025

@Shaunak01
Is there the way to fast disabling there warnings:
image

It's really hard to read the code between these warnings.

Copy link
Contributor

@dremdem dremdem left a comment

Choose a reason for hiding this comment

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

Looking good.
Some questions.

Comment on lines +71 to +74
# After the test run, rename the generated coverage.xml to coverage_fast.xml.
- name: Rename Fast Test Coverage report
if: always()
run: mv ./coverage.xml ./coverage_fast.xml
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering, why we need it if we will specify the flags: fast in the next step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if you look at the invoke function in helpers/lib_tasks_pytest.py. Initially we had combined coverage only now i have introduced separate fast and slow coverage runs. But both generate the same file coverage.xml due to internal logic of porting. If i rename in the internal logic, we have to debug some porting logic as codecov is not picking up other files. So a better solution is

Generate Fast test coverage - coverage.xml -> rename 'coverage_fast.xml'-> upload
Generate Slow test coverage - coverage.xml -> rename 'coverage_slow.xml'-> upload

Codecov combines so we dont need to.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I got the point. To be honest, I'm lost.
@Shaunak01 Let's chat tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works!

fail_ci_if_error: true

# This step is used to trigger the slow test coverage generation using the invoke task.
- name: Run Slow test and generate report
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably, we need separate workflows for the fast, slow and super slow tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous comment answers this

Comment on lines +885 to +888
:param target_dir: directory for coverage stats. Use "." to indicate all directories
:return: tuple (include_in_report, exclude_from_report) where:
- include_in_report is a glob pattern for files to include
- exclude_from_report is a comma-separated glob pattern for files to exclude (or None if not applicable)
Copy link
Contributor

Choose a reason for hiding this comment

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

Reflow

@@ -864,6 +865,8 @@ def run_coverage_report( # type: ignore
if exclude_from_report is not None:
report_html_cmd += f" --omit={exclude_from_report}"
report_cmd.append(report_html_cmd)
# Generate an XML report for Codecov.
report_cmd.append("coverage xml -o coverage.xml")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we disable these comments

image

from Codecov?

I think they are cool, but for now they add a lot of noise.

@Shaunak01
Copy link
Contributor Author

closing with follow up PR at: #598

@Shaunak01 Shaunak01 closed this Apr 22, 2025
@Shaunak01 Shaunak01 deleted the HelpersTask487_Add_Git_actions_for_Codecov_3 branch April 22, 2025 14:15
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