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

tests: add a test case mark tool #39440

Closed
wants to merge 11 commits into from
Closed

Conversation

mahjonp
Copy link
Contributor

@mahjonp mahjonp commented Nov 29, 2022

What problem does this PR solve?

This PR adds a check to verify that incremental test cases are annotated with the associated Feature ID or Issue ID. For example, a test case should have the following form:

func TestBindingWithoutCharset(t *testing.T) {
    // Used to mark a Feature
    marker.As(t, maker.Feature, "FD-1024", "balabala")
    // Or no FD is assigned
    marker.As(t, maker.Feature, maker.NoID)
    // Or used to mark an Issue
    marker.As(t, maker.Issue, 1234)
    ...
}

Issue Number: close #xxx

Problem Summary:

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 29, 2022
@mahjonp mahjonp force-pushed the mark_feature branch 2 times, most recently from 97da1b6 to 145a8fc Compare November 30, 2022 02:54
@mahjonp mahjonp force-pushed the mark_feature branch 2 times, most recently from 4f321b4 to d6f54c9 Compare December 8, 2022 08:12
@mahjonp mahjonp changed the title [DNM] tests: add a test case feature mark tool tests: add a test case mark tool Dec 8, 2022
@mahjonp mahjonp force-pushed the mark_feature branch 2 times, most recently from 70ca79a to 33fc594 Compare December 12, 2022 05:07
Makefile Outdated Show resolved Hide resolved
DEPS.bzl Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
tests/testmarker/BUILD.bazel Show resolved Hide resolved
@mahjonp mahjonp force-pushed the mark_feature branch 5 times, most recently from 78220bc to 993555c Compare December 14, 2022 03:12
Signed-off-by: mahjonp <junpeng.man@gmail.com>
Signed-off-by: mahjonp <junpeng.man@gmail.com>
Signed-off-by: mahjonp <junpeng.man@gmail.com>
Signed-off-by: mahjonp <junpeng.man@gmail.com>
Signed-off-by: mahjonp <junpeng.man@gmail.com>
@mahjonp mahjonp force-pushed the mark_feature branch 3 times, most recently from 6c0464a to ee3f414 Compare December 14, 2022 11:17
Signed-off-by: mahjonp <junpeng.man@gmail.com>
@TennyZhuang
Copy link
Contributor

The implementation looks stupid enough, and introduce too many invasions for developers. If you are interested in some metrics about fine-grained test coverage, why not associate the incremental tests with the commit, and related issue?
An incremental test can also be found automatically by scanning all tests before and after a specified commit, then diff them.

@mahjonp
Copy link
Contributor Author

mahjonp commented Dec 15, 2022

The implementation looks stupid enough, and introduce too many invasions for developers. If you are interested in some metrics about fine-grained test coverage, why not associate the incremental tests with the commit, and related issue? An incremental test can also be found automatically by scanning all tests before and after a specified commit, then diff them.

Sorry for the invasions to the developers, what you said makes sense. I will look into whether the current R&D process is complete and if it can index a complete set of PRs from the feature id. @TennyZhuang

@mahjonp
Copy link
Contributor Author

mahjonp commented Dec 15, 2022

The implementation looks stupid enough, and introduce too many invasions for developers. If you are interested in some metrics about fine-grained test coverage, why not associate the incremental tests with the commit, and related issue? An incremental test can also be found automatically by scanning all tests before and after a specified commit, then diff them.

Sorry for the invasions to the developers, what you said makes sense. I will look into whether the current R&D process is complete and if it can index a complete set of PRs from the feature id. @TennyZhuang

By the way, this PR is planned to use nogo config to control the directories or files that are checked in alpha testing (for example starting from the sql team first), collect feedback and improve during the practice, and will not be released all at once. It is considered to minimize the intrusion to developers.

@mahjonp mahjonp marked this pull request as draft December 15, 2022 03:00
@ti-chi-bot ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 15, 2022
@mahjonp mahjonp marked this pull request as ready for review December 15, 2022 05:05
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 15, 2022
"/external/": "no need to vet third party code"
},
"only_files": {
".*_test\\.go$": "ignore test code"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
".*_test\\.go$": "ignore test code"

will remove later and select some dirs for alpha testing

@mahjonp
Copy link
Contributor Author

mahjonp commented Dec 15, 2022

After survey, I found that the current R&D process is unable to link the Feature ID(create on Jira) to a GitHub PR or issue, and that there is no feature id information attached to the issue or PR. Therefore, adding a marker may be the only current solution, at least for marking FD-$ID for feature development PR 😰

@mahjonp
Copy link
Contributor Author

mahjonp commented Dec 15, 2022

/rebuild

@mahjonp
Copy link
Contributor Author

mahjonp commented Dec 15, 2022

/run-build

Signed-off-by: mahjonp <junpeng.man@gmail.com>
@mahjonp
Copy link
Contributor Author

mahjonp commented Dec 16, 2022

/run-build

Signed-off-by: mahjonp <junpeng.man@gmail.com>
@mahjonp
Copy link
Contributor Author

mahjonp commented Dec 16, 2022

/run-build

Signed-off-by: mahjonp <junpeng.man@gmail.com>
@mahjonp
Copy link
Contributor Author

mahjonp commented Dec 16, 2022

/run-all-tests

@TennyZhuang
Copy link
Contributor

TennyZhuang commented Dec 16, 2022

Please also update CONTRIBUTION.md

Bug fixes usually come with tests. With the help of continuous integration test, patches can be easy to review. Please update the unit tests so that they catch the bug! Please check example #2808.

A community contributor can't add a test now, so please remove these lines.

Some examples of "Architecture" improvements:
Improving test coverage.

ditto

Make commits of logical units and add test case if the change fixes a bug or adds new functionality.

ditto


Another simple solution: Please remove the fake CONTRIBUTING.md.

@mahjonp
Copy link
Contributor Author

mahjonp commented Dec 16, 2022

close because of controversy

@mahjonp mahjonp closed this Dec 16, 2022
@mahjonp mahjonp deleted the mark_feature branch December 16, 2022 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants