-
Notifications
You must be signed in to change notification settings - Fork 11
Added support for LAVA framework including YAML files #30
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
Conversation
vnarapar
commented
May 14, 2025
- Integrated LAVA framework with new YAML plans
- Added send-to-lava.sh script with SPDX license header
- Fixed indentation issues across all shell scripts
- Replaced 'source' with '.' in all scripts for consistency
- Removed redundant variables in YAML files
- Cleaned up bash syntax for better readability and maintainability
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.
@vnarapar feel free to remove my s-o-b from commits. I didn't do any work here. The signoffs are there after doing a cleanup and resolving conflicts. I leave the decision to you.
The commit messages still don't explain why changes are made. Please update it to reflect the reasons.
Please change the copyright on send-to-lava.sh as requested by @craigez
Runner/utils/send-to-lava.sh
Outdated
@@ -0,0 +1,58 @@ | |||
#Copyright: 2016-2019 Linaro Limited |
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.
As explained by Craig, this should go away and be replaced with URL to the original source
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.
Can you please check if the below looks fine. so that i can add the same?
Upstream-Contact: Linaro validation mailing list [email protected]
Script is from https://github.com/Linaro/test-definitions
SPDX-License-Identifier: GPL-2.0-only
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.
Also as requested removed your s-o-b and updated the commits on why they are made. Will update the license aswell if the above looks good.
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 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 think this is good for that file. We also should include the details in the LICENSE file too, so it appears at the top level and notice obligations can be met. There is where we can include Linaro copyrights from their project. Since they don't differentiate which copyright holders specifically contributed to this file, we might need to credit them all, for example:
Runner/utils/send-to-lava.sh:
Script is from https://github.com/Linaro/test-definitions
Copyright: 2012-2019 Linaro Limited
2019 Daniel Wagner <[email protected]>
2019 Daniel Wagner <[email protected]>
2019 Patryk Mungai <[email protected]>
2018 Karsten Tausche <[email protected]>
2018 Oleksandr Terentiev <[email protected]>
2017 Lei Yang <[email protected]>
SPDX-License-Identifier: GPL-2.0-only
I also want to double check with @nbobbaqcom on some other requirements
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 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.
@craigez @nbobbaqcom thanks for your help
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.
Hi @mwasilew @nbobbaqcom @craigez
Added the license as suggested. please help review
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.
Looks good to me.
Please check this explanation #14 (comment) and fix it. |
0bd1ded
to
1fec25f
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.
Please change commit messages to use passive voice.
I'm not sure we need to include full GPL license in the LICENSE file. It can stay if it's a requirement.
Overall I think we're very close to merging this. There are only minor details left.
Runner/plans/KernelCI_PreMerge.yaml
Outdated
run: | ||
steps: | ||
- cd Runner | ||
- chmod -R 777 * |
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 should not be here. Proper file permissions should be set in the repository.
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.
removed the line and added permissions to the repository
Runner/plans/meta-qcom_PreMerge.yaml
Outdated
run: | ||
steps: | ||
- cd Runner | ||
- chmod -R 777 * |
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 should not be 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.
removed the line and added permissions to the repository
fi | ||
echo "$TESTNAME PASS" > $test_path/$TESTNAME.res | ||
echo "$TESTNAME FAIL" > $test_path/$TESTNAME.res |
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 looks wrong. The test will always fail. Is this expected?
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 test will only fail if line#53 is executed which is the expectations as the condition is not met
|
||
validate_cpu_core() { | ||
local cpu=$1 | ||
local core_id=$2 | ||
cpu=$1 |
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.
using local is indeed undefined in /bin/sh, but it's a common practice to use it as it helps defining variable scope. Maybe we should keep it.
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 local variables as suggested
exit 1 | ||
<<<<<<< HEAD |
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.
Please remove merge conflict marker and the remaining fi
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 is removed as part of other commit
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.
You don't need 755 permissions on LICENSE or .yaml files.
b462a14
to
c627369
Compare
Integrated LAVA framework with new YAML plans Added send-to-lava.sh script with SPDX license header Removed redundant variables in YAML files Removed Audio_Premerge yaml file Copied send-to-lava.sh from linaro to support result parsing with LAVA framework. Original source: https://github.com/Linaro/test-definitions/blob/master/automated/utils/send-to-lava.sh License: GPL-2.0-only, MIT This change is added for LAVA framework support to all the scripts based on testplan. Added the license file with 644 permissionsin the same folder where send-to-lava.sh is present as per review comment Removed chmod permissions in testplansand gave permissions to repository Signed-off-by: Vamsee Narapareddi <[email protected]>
Replaced 'source' with '.' in all scripts for consistency Cleaned up bash syntax for better readability and maintainability Signed-off-by: Vamsee Narapareddi <[email protected]>
Modified the function find_test_case_by_name and run_specific_test_by_name in run-test.sh to find the testcase to execute easily without depending on find command Removed unwanted lines from run-test.sh as it is dead code Signed-off-by: Vamsee Narapareddi <[email protected]>
Fixed all the indentation issues in all shell scripts for consistancy Signed-off-by: Vamsee Narapareddi <[email protected]>
Made few changes for the script to work for sh. The previous change works for bash. Hence modified to work with sh Added back local variables as per review comment Signed-off-by: Vamsee Narapareddi <[email protected]>
Removed logging related functions at testcase level since it is duplicate code and already present in functestlib.sh Added functestlib.sh for IPA and RMNET as logging related functions log_pass/log_fail are defined in functestlib.sh Signed-off-by: Vamsee Narapareddi <[email protected]>
Removed prev results Integrated LAVA framework with new YAML plans Added send-to-lava.sh script with SPDX license header Removed redundant variables in YAML files Integrated LAVA framework with new YAML plans Added send-to-lava.sh script with SPDX license header Removed redundant variables in YAML files This commit will remove few merge conflits Signed-off-by: Vamsee Narapareddi <[email protected]>
@mwasilew added 644 to License and 744 is required for yaml files to execute from LAVA |
Merge PR #30 Signed-off-by: Milosz Wasilewski <[email protected]>
There was one more change I made manually to avoid extending this review. It's now merged to main branch. Closing this PR. |