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

feat(tiflow): upload tiflow ut codecoverage if test passed #2739

Merged

Conversation

purelind
Copy link
Collaborator

@purelind purelind commented Jan 10, 2024

  • upload tiflow ut codecoverage if test passed
  • upload code coveage in master and lts branch's pull requests

Copy link

ti-chi-bot bot commented Jan 10, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:

The pull request adds the feature to upload tiflow ut code coverage if tests pass and upload code coverage in master and lts branch. The changes involve updating environment variables and adding a script to upload code coverage.

Potential problems:

  1. The pull request only adds the feature to upload code coverage to the tiflow pipeline. It might be necessary to add this feature to other pipelines as well.
  2. The TICDC_CODECOV_TOKEN and TICDC_COVERALLS_TOKEN environment variables are not used anymore. If these variables are used elsewhere, it might cause issues in other parts of the application.

Fixing suggestions:

  1. If uploading code coverage is needed in other pipelines, it might be necessary to create a common function to upload code coverage and use it across all pipelines.
  2. If the TICDC_CODECOV_TOKEN and TICDC_COVERALLS_TOKEN environment variables are used elsewhere, it might be necessary to update these variables to use the CODECOV_TOKEN.

Overall, the changes seem straightforward and do not introduce any major issues.

@ti-chi-bot ti-chi-bot bot added the size/L label Jan 10, 2024
Copy link

ti-chi-bot bot commented Jan 10, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.
Pull Request Review

The pull request adds a new feature to upload tiflow ut code coverage if the test passed. The key changes are adding the CODECOV_TOKEN environment variable and a new script to upload the coverage results.

However, there are some potential problems that need to be addressed:

  • The description of the pull request is not descriptive enough. It would be helpful to provide more context about the feature being added.
  • The script to upload the code coverage results seems to be repeated in every branch. It may be better to create a separate script and include it in each branch rather than repeating the same code.
  • It is not clear what happens if the tests fail. The script only runs if the tests pass. It would be helpful to include an error message or notification if the tests fail.

Fixing Suggestions:

  • Provide a more descriptive pull request description that details what the feature is and how it works.
  • Create a separate script to upload the code coverage results and include it in each branch.
  • Add an error message or notification if the tests fail.

@purelind
Copy link
Collaborator Author

we need to remove the upload step in the tiflow makefile. pingcap/tiflow#10446

Comment on lines 91 to 93
dm_unit_test_in_verify_ci: [flags: "unit,dm", coverage_file: "/tmp/dm_test/cov.unit_test.out"],
unit_test_in_verify_ci: [flags: "unit,cdc", coverage_file: "/tmp/tidb_cdc_test/cov.unit.out"],
engine_unit_test_in_verify_ci: [flags: "unit,engine", coverage_file: "/tmp/engine_test/cov.unit_test.out"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ref:

Let's migrate to use flags to separate test types. components will be identified auto by file path defined in repo's codecov.yaml file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does this PR pingcap/tiflow#9252 need to be cherry-picked to other LTS branches?

@wuhuizuo
Copy link
Collaborator

  • upload tiflow ut codecoverage if test passed
  • upload code coveage in master and lts branch

But it will just upload coverage for pull request, we also need another postsubmit job to upload coverage to base branch

Copy link

ti-chi-bot bot commented Jan 11, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.
Pull Request Review

Key Changes:

  • The PR is adding code to upload tiflow ut codecoverage if the test has passed.
  • It is uploading code coverage in master and lts branch's pull requests.

Potential Problems:

  • It is not clear what prow.uploadCoverageToCodecov method does. It is not defined in the given code, so it may throw an error.
  • The environment variable names have been changed from TICDC_CODECOV_TOKEN and TICDC_COVERALLS_TOKEN to CODECOV_TOKEN. This may cause a problem if these variables are being used somewhere else in the code.
  • It is not clear if the code coverage is being uploaded to the correct location or not.

Fixing Suggestions:

  • Define the method prow.uploadCoverageToCodecov if it is not already defined.
  • Use a different name for the environment variables that are not already being used in the code.
  • Add comments to the code to make it clear what prow.uploadCoverageToCodecov method does and what parameters it takes.
  • Verify if the code coverage is being uploaded to the correct location or not.

@ti-chi-bot ti-chi-bot bot added size/M and removed size/L labels Jan 11, 2024
Copy link

ti-chi-bot bot commented Jan 11, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the lgtm label Jan 11, 2024
Copy link

ti-chi-bot bot commented Jan 11, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-01-11 09:24:48.819324162 +0000 UTC m=+521678.403577869: ☑️ agreed by wuhuizuo.

@ti-chi-bot ti-chi-bot bot added the approved label Jan 11, 2024
@ti-chi-bot ti-chi-bot bot merged commit 3207a70 into PingCAP-QE:main Jan 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants