-
Notifications
You must be signed in to change notification settings - Fork 22
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
CNDB-9877: Update PR template #1399
base: main
Are you sure you want to change the base?
Conversation
Quality Gate passedIssues Measures |
This is a test comment 2024-10-31T14:11:20.585555532Z |
@@ -5,12 +5,12 @@ | |||
... | |||
|
|||
### Checklist before you submit for review | |||
- [ ] Make sure there is a PR in the CNDB project updating the Converged Cassandra version | |||
- [ ] Make sure there is a PR in the CNDB project updating the Converged Cassandra version if your PR is changing the API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- [ ] Make sure there is a PR in the CNDB project updating the Converged Cassandra version if your PR is changing the API | |
- [ ] Make sure there is a PR in the CNDB project updating the Converged Cassandra version if your PR affects CNDB. |
The question is, how do we know the PR affects CNDB? For example it could be a HCD change and indirectly affect CNDB. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't a similar problem as we have with Stargate-CNDB relation? Stargate uses CNDB coordinator as an impl so we run stargate internal tests against that implementation;
I think the bare minimum would be to compile CNDB against the CC artifact produced in from the feature branch as a step in CC gating
- [ ] Each commit contains related changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- [ ] Each commit contains related changes | |
- [ ] Each commit is specific to the issue it's addressing |
- [ ] Each commit contains related changes | ||
- [ ] Renames, moves and reformatting are in distinct commits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still drop this one, as I think most of the time it would generate more - artificially separated - work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can drop it, but it is just a friendly reminder - personally I'd not start reviewing a PR which mixes renamings and major reformattings with the code changes - just with "how to write tests" - it is just sharing "how to"
- [ ] Each commit contains related changes | ||
- [ ] Renames, moves and reformatting are in distinct commits | ||
- [ ] Avoid starting nodes in new tests (jvm dtests, CQLTester) if possible in favor of implementing pure unit tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite agree with this: in a complex distributed system, integration tests are fundamental and should not be discouraged even if unit tests exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though starting nodes over and over again should be avoided - if we are adding a test case to the test class which already has some shared containers running - that's fine; however we should design tests having the testing time in mind;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but this is more of a general "know how to write efficient tests" issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed; though PR template seems to be an efficient way to share that knowledge to the most appropriate audience, isn't it?
What is the issue
n/a
What does this PR fix and why was it fixed
n/a
Checklist before you submit for review
NoSpamLogger
for log lines that may appear frequently in the logs