PingCAP-QE / ci

Continue intergration tests
Apache License 2.0
19 stars 97 forks source link

fix(br): use failpoint tidb-server instead #2951

Closed purelind closed 2 months ago

purelind commented 2 months ago

Since https://github.com/pingcap/tidb/pull/52734, we need to use failpoint tidb-server for br integration test

ti-chi-bot[bot] commented 2 months ago

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

Based on the pull request description and the provided diff, it seems that the changes are related to using a different failpoint for the integration test of br. Specifically, the change is to use failpoint tidb-server instead of some other failpoint that was previously being used.

There do not seem to be any potential problems with this change. However, some suggestions for fixing the code could be:

ti-chi-bot[bot] commented 2 months ago

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

Based on the PR title and description, it seems that the changes are related to using a different failpoint for the integration test in qa-release-br-integration-test.groovy. The change is to use failpoint tidb-server instead of the previous one.

The diff shows that the code changes are minimal and only add a new make command for building the binary for br.test. However, there are a few potential problems with this pull request:

  1. It's not clear why the previous failpoint was not working and why this new one is better.
  2. The code changes do not include any tests, so it's unclear if this change will break any existing functionality.
  3. The new make command could increase the build time for the pipeline.

To fix these issues, I suggest the following:

  1. The PR author should provide more context on why the previous failpoint was not working and why this new one is better.
  2. The PR author should add some tests to ensure that this change does not break any existing functionality.
  3. The new make command could be moved to a separate step in the pipeline, so it does not slow down the main build process.
ti-chi-bot[bot] commented 2 months ago

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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: - **[pipelines/OWNERS](https://github.com/PingCAP-QE/ci/blob/main/pipelines/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment