-
Notifications
You must be signed in to change notification settings - Fork 309
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
DAOS-12950 test: auto-determine test tags #13315
Conversation
Bug-tracker data: |
acf6905
to
6c5a703
Compare
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.
LGTM. No errors found by checkpatch.
Test stage Python Bandit check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13315/7/execution/node/148/log |
6c5a703
to
5859040
Compare
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.
LGTM. No errors found by checkpatch.
Test stage Python Bandit check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13315/8/execution/node/149/log |
5859040
to
57b4e09
Compare
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.
LGTM. No errors found by checkpatch.
Test stage Functional on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13315/9/execution/node/749/log |
57b4e09
to
d08f925
Compare
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.
LGTM. No errors found by checkpatch.
Test stage Functional on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13315/10/execution/node/749/log |
d08f925
to
bd1d1df
Compare
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.
LGTM. No errors found by checkpatch.
Test stage Functional on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13315/11/execution/node/764/log |
bd1d1df
to
ebfae55
Compare
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.
LGTM. No errors found by checkpatch.
ebfae55
to
cd07133
Compare
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.
LGTM. No errors found by checkpatch.
Skip-func-hw-test: true Skip-unit-tests: true Skip-fault-injection-test: true - Add a tool to: - automatically determine test tags based on files modified. - run a lint check on ftest tags - Add a GHA job for runnings ftest tag lint - Update Jenkinsfile Get Commit Message stage to inject Test-tag from the new utility Required-githooks: true Signed-off-by: Dalton Bohning <[email protected]>
cd07133
to
c6f6acb
Compare
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.
LGTM. No errors found by checkpatch.
@@ -326,7 +326,7 @@ pipeline { | |||
stage('Get Commit Message') { | |||
steps { | |||
script { | |||
env.COMMIT_MESSAGE = sh(script: 'git show -s --format=%B', | |||
env.COMMIT_MESSAGE = sh(script: 'ci/get_commit_message.py --target origin/' + target_branch, |
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.
The reason for doing it this way is to prevent needing to modify pipelinelib.
.github/workflows/linting.yml
Outdated
with: | ||
ref: ${{ github.event.pull_request.head.sha }} | ||
- name: Check DAOS ftest tags. | ||
run: ./src/tests/ftest/tags.py lint |
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.
This needs to be protected against PRs where this check has landed but PRs aren't including it yet. I can send you code for this.
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 follow. Do GHA for PRs use the master workflows?
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.
Added a check
ci/get_commit_message.py
Outdated
return os.linesep.join(commit_message_split) | ||
|
||
|
||
if __name__ == '__main__': |
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.
Code after this should be in a main() or similar function. That avoids namespace issues with the "locals" in this code block and any surprises with global variables.
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.
Done
debian/changelog
Outdated
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 know why this file is needed in the RPM but I'd back out this change for now to avoid conflicts.
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.
Generally, it would useful to have this utility available to probe for things like "what tags do I need to run this test"
src/tests/ftest/tags.py
Outdated
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.
Would this file be better in ci
or similar? It's purpose is to choose what to run in CI from a PR and inherently by the time you have a RPM it's too late to call it, plus it's not really part of ftest, it's something that's used to drive ftest and frankly this directory is already crowded and a bit confusing.
One advantage of that would be you could call it from the jira_comment.py script and comment to the PR what test-tags it thinks are appropiate.
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.
Maybe it doesn't belong in ftest
, but I wouldn't say it belongs in ci
. It's written to be useful locally as well, and we would like to expand functionality for things other than CI. I don't think the core functionality should be tightly coupled with our CI infrastructure.
It's purpose is to choose what to run in CI from a PR
That's not exactly true. That's the purpose of ci/get_commit_message.py
. The purpose of ftest/tags.py
is to provide an interface to the functional test (ftest) tags. All of these tags are avocado tags, and we only use avocado in ftest. In fact, we can't even properly figure out the tags without parsing through test_*
methods. So the tags are rightly coupled with the ftest structure.
One advantage of that would be you could call it from the jira_comment.py script and comment to the PR what test-tags it thinks are appropiate
Do you mean jira_query.py? 1) That only comments once AFAIK, so it wouldn't be useful for subsequent commits. 2) It would be noisy to comment for every commit.
But yes, part of the remaining work here is to somehow propagate the tags to the PR/owner
All this said, I'm open to moving this file to somewhere that makes more sense. I'm just not sure where that is
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 like the idea of an API to query the test tags from something, be that a commit message, a string or a target sha etc. I think that's what this file is but if that's the case then it needs to be better documented and probably a clearer interface. I'm also sceptical if re is required or just split(:) would suffice.
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'm also sceptical if re is required or just split(:) would suffice.
To rely on just split(':')
we would need logic to loop over each line and match lines starting with Test-tag:
and Features:
(and case insensitive). Which basically means writing a simplified version of our own re
. I don't think that's the way to go.
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.
There's a function to do it in https://github.com/daos-stack/daos/pull/8483/files but it's really very simple.
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.
For reference, this is with re
import re
features = re.findall(r'Features: (.*)', message, flags=re.MULTILINE | re.IGNORECASE)
test_tags = re.findall(r'Test-tag: (.*)', message, flags=re.MULTILINE | re.IGNORECASE)
And this is manually:
features = []
test_tags = []
for line in message.lower().splitlines():
if line.startswith('features:'):
features.append(line.split(':', maxsplit=1)[1])
if line.startswith('test-tag:'):
test_tags.append(line.split(':', maxsplit=1)[1])
Personally, I think re
is more readable and more extensible
Definitely. This whole thing warrants a discussion in the PDT, and the idea is that developers could and should contribute to that list |
Some TODO items here
|
Required-githooks: true Signed-off-by: Dalton Bohning <[email protected]>
Signed-off-by: Dalton Bohning <[email protected]>
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.
LGTM. No errors found by checkpatch.
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.
LGTM. No errors found by checkpatch.
- Merge Features and Test-tag since both are not supported - If commit has Test-tag, just leave a comment with the recommended Signed-off-by: Dalton Bohning <[email protected]>
Test stage NLT on EL 8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-13315/16/display/redirect |
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.
LGTM. No errors found by checkpatch.
if test_tags: | ||
# Keep the commit tags but leave a comment with the recommended | ||
commit_message_split.insert(2, f'Test-tag: {" ".join(sorted(commit_tags))}') | ||
commit_message_split.insert(3, '# Auto-recommended Test-tag') | ||
commit_message_split.insert(4, f'# Test-tag: {" ".join(sorted(rec_tags))}') | ||
commit_message_split.insert(5, '') | ||
else: | ||
# If there were "Feature" tags, merge into the recommended. | ||
# Otherwise, just use the recommended. | ||
rec_tags.update(commit_tags) | ||
commit_message_split.insert(2, '# Auto-recommended Test-tag') | ||
commit_message_split.insert(3, f'Test-tag: {" ".join(sorted(rec_tags))}') | ||
commit_message_split.insert(4, '') |
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.
Still need to shake out the details but the idea is
- Features and Test-tag are not both supported. If the commit has both, merge as a convenience. I've seen people try to specify both.
- If the commit has Test-tag, just leave a comment with the recommended. This allows people to override for whatever reason. E.g. when trying iterating on a broken test and not wanting to run everything for every commit.
The end result is that the modified commit never has Features. Only Test-tag
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.
When you say "supported" this is all our code I assume, there's some logic that we own in launch.py that extracts this and passes it to the avocado command line?
I'm not saying this is wrong or requesting changes but just trying to understand the requirements and limitations.
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.
No. pipeline-lib extracts commit pragmas and converts the Features and Test-tag to a single line that gets passed to launch.py
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.
Well, pipeline-lib is also within our control but harder to modify than here certainly.
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.
Why is it harder to modify? It even tests the modifications being made with all currently supported branches to try to prevent unexpected regressions.
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.
It's a separate repo, so I'd have to first update pipeline lib and verify it's working
Yes. Always have to verify changes being made work as intended. But this isn't really any different than if it were on the same project.
and backwards compatible with other branches.
pipeline-lib
's CI does that for you. It tests the pipeline-lib
against daos' master
, release/2.4
, weekly-testing
and weekly-2.4-testing
branches. Might want to think about adding the new daily*
branches in there but just those 4 branches is pretty good coverage. You can even define a Test-tag:
you want to be used during all of that testing.
And I would still need a daos PR to verify expected behavior of passing both pragmas.
Again, that sounds like it's just a normal process of testing/validating one's changes, regardless of which repo they are in.
Anyway, just hoping to clear up any misconceptions on the perceived difficulty of updating pipeline-lib
.
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.
Yes. Always have to verify changes being made work as intended. But this isn't really any different than if it were on the same project.
- By using just the daos repo, I can run and debug some code locally to verify it's working. Testing pipeline-lib code locally is virtually impossible.
- I would need TWO PRS - one in pipeline-lib, and then one in daos that uses the pipeline-lib one. And every time I update the pipeline-lib PR, I would have to retrigger my daos PR.
And I would still need a daos PR to verify expected behavior of passing both pragmas.
Again, that sounds like it's just a normal process of testing/validating one's changes, regardless of which repo they are in.
I disagree. If I have a daos PR, I only need ONE PR, and I only need to verify my changes on that PR for that ONE branch. With pipeline-lib, I would have to verify all supported branches. Sure, pipeline-lib has automated steps for some of this, but it's still more complex that just ONE daos PR. And I would still have to manipulate the Test-tag
and Features
commit pragmas for those auto-spawned jobs to verify it works. AND I would still need a separate daos PR to verify it works in "PR mode", since pipeline-lib has different behavior depending on whether it's a timed build, PR, upstream, etc. AND it takes much longer to wait for testing from 4 branches than for one branch.
Another benefit to having just a daos PR is that the change is atomic. One PR, one commit - instead of co-dependent PRs in both daos and pipeline-lib.
In all, I find pipeline-lib incredibly cumbersome to work with.
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.
In all, I find pipeline-lib incredibly cumbersome to work with.
Not disagreeing that it's certainly more cumbersome than having everything in a single repo on a single branch. But then when you do branch you have to start duplicating changes across branches. The exact reason libraries were invented.
That said, library is not really even our primary reason for pipeline-lib
(as much as we do benefit from it being a common library to many projects and branches and greatly reducing duplication). It's primary reason for existence is Jenkins itself and it's limitations. We've had to move soooo much code out of a daos branch's Jenkinsfile
into pipeline-lib
just because there is an upper limit to the size of Jenkinsfile
.
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.
But then when you do branch you have to start duplicating changes across branches. The exact reason libraries were invented.
But proper libraries are versioned and you can specify which version you want to use. With pipeline-lib, all daos branches are forced to use the same version. Which goes back to the previous problem of having to make sure all active branches still work.
I don't think putting logic in the daos repo is duplicating code - it's just versioned for different branches. We don't want to do the exact same thing for every branch.
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.
there is an upper limit to the size of Jenkinsfile
FWIW I don't think it's the Jenkinsfile that has a limitation on size. It's the pipeline
object that is limited in size. But either way, this is an external python script and doesn't affect that.
I.e. when this PR merges to master but branches don't have the changes yet Required-githooks: true Signed-off-by: Dalton Bohning <[email protected]>
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.
LGTM. No errors found by checkpatch.
Test stage Functional on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13315/18/execution/node/1157/log |
.github/workflows/linting.yml
Outdated
- name: Checkout code | ||
uses: actions/checkout@v4 | ||
with: | ||
ref: ${{ github.event.pull_request.head.sha }} |
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.
Normal behaviour is to checkout a merge commit with the target branch, this code will checkout the branch itself.
The difference comes in annotating against line numbers, with a merge commit they might be wrong although of course with a branch checkout you're not testing what would be landed. Unless you're annotating the PR against specific lines in files you probably don't want this, if you're just failing the build with logging or annotating against files (which I think is done against "line 0" then you don't want to specify a ref here.
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.
Thanks. The ftest lint just fails with logging + exit(1). No specific lines or files are annotated. So I think I want the default behavior
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.
There's a function to do it in https://github.com/daos-stack/daos/pull/8483/files but it's really very simple.
commit_message = re.sub( | ||
r'^ *Test-tag:.*$', '', commit_message, flags=re.MULTILINE | re.IGNORECASE) | ||
|
||
# Put "Test-tag" after the title |
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've always put them at the end. Put the human-readable bit at the beginning where it can be seen and the machine readable bit at the end out of the way.
Like Dalton says however this doesn't appear to matter but it does highlight that the way this code is communicating with ftest is something we might be able to improve.
if test_tags: | ||
# Keep the commit tags but leave a comment with the recommended | ||
commit_message_split.insert(2, f'Test-tag: {" ".join(sorted(commit_tags))}') | ||
commit_message_split.insert(3, '# Auto-recommended Test-tag') | ||
commit_message_split.insert(4, f'# Test-tag: {" ".join(sorted(rec_tags))}') | ||
commit_message_split.insert(5, '') | ||
else: | ||
# If there were "Feature" tags, merge into the recommended. | ||
# Otherwise, just use the recommended. | ||
rec_tags.update(commit_tags) | ||
commit_message_split.insert(2, '# Auto-recommended Test-tag') | ||
commit_message_split.insert(3, f'Test-tag: {" ".join(sorted(rec_tags))}') | ||
commit_message_split.insert(4, '') |
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.
When you say "supported" this is all our code I assume, there's some logic that we own in launch.py that extracts this and passes it to the avocado command line?
I'm not saying this is wrong or requesting changes but just trying to understand the requirements and limitations.
@@ -0,0 +1,127 @@ | |||
#!/usr/bin/env python3 |
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.
TODO: add copyright
Required-githooks: true Signed-off-by: Dalton Bohning <[email protected]>
Skip-func-hw-test: true Skip-unit-tests: true Skip-fault-injection-test: true Required-githooks: true Signed-off-by: Dalton Bohning <[email protected]>
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.
LGTM. No errors found by checkpatch.
Test stage Functional on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13315/19/execution/node/764/log |
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.
Now that we have the GHA alternative to running functional tests, which exists primarily (at this point at least) for external contributors, who would benefit the most from this concept, we should include this functionality in those test runs.
Perhaps it's as simple as changing
git show -s --format=%B | escape_single_quotes >> $GITHUB_OUTPUT; |
Line 322 in 188afe6
env.COMMIT_MESSAGE = sh(script: 'ci/get_commit_message.py --target origin/' + target_branch, |
Ah, yes! I will do that once we've settled on the interface between the Jenkinsfile and utility |
git_root = git_root_dir() | ||
result = subprocess.run( | ||
['git', 'diff', target, '--name-only', '--relative'], | ||
stdout=subprocess.PIPE, cwd=git_root, check=True) | ||
return [os.path.join(git_root, path) for path in result.stdout.decode().split('\n') if path] |
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.
This isn't correct in the case where the branch isn't up-to-date with the target. It needs logic similar to find_base.sh
Skip-func-hw-test: true Skip-unit-tests: true Skip-fault-injection-test: true Required-githooks: true Signed-off-by: Dalton Bohning <[email protected]>
I've started a simpler version of this work in #13642. It adds most of the core utilities and a linter, but doesn't change the commit message yet. I'd like to get that through first, then we can focus on the details of the interface here. |
Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13315/20/execution/node/763/log |
closed in favor of #13964 |
Skip-test: true
Skip-build: true
Required-githooks: true
Before requesting gatekeeper:
Features:
(orTest-tag*
) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.Gatekeeper: