Skip to content

Conversation

avisinone
Copy link

No description provided.

@avisinone

This comment was marked as outdated.

@smuppand
Copy link
Contributor

smuppand commented May 27, 2025

@avisinone Noticed that a few commits are duplicated in this PR (the same changes appear multiple times in the commit history). Please consider rebasing or squashing to remove the duplicates for a cleaner history. Also provide commit message. Also your run.sh is not aligned to run in the testkit suite. Refer https://github.com/smuppand/qcom-linux-testkit/blob/lib_changes/CONTRIBUTING.md

@mwasilew
Copy link
Contributor

Rebasing your branch on top of main will fix the license issue. Last commit (e4bda8ffad71b14d891e46ec38e1985e85883bcb) in your branch is duplicated from main branch and that causes the problem.

Copy link
Contributor

@smuppand smuppand left a comment

Choose a reason for hiding this comment

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

rebase your feature branch on top of latest main.
Unify PASS/FAIL results handling to reduce redundancy.
Along with left few more comments.

@avisinone avisinone requested a review from smuppand June 5, 2025 12:41
Copy link
Contributor

@mwasilew mwasilew left a comment

Choose a reason for hiding this comment

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

Please use indentation consistently and rewrite the commit message.

@avisinone avisinone force-pushed the platform_test branch 2 times, most recently from 0cb77ff to 6bbb509 Compare June 10, 2025 10:44
@avisinone avisinone requested a review from mwasilew June 10, 2025 10:46
update_test_fail(){
subtestname=$1
msg=$2
echo "$subtestname FAIL" >> "$res_file"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please only add Over PASS/Fail status. Since this result file is getting parsed to get the test result, it might give wrong status?

Copy link
Author

Choose a reason for hiding this comment

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

Hi I have changed the result logic. please check

@avisinone avisinone force-pushed the platform_test branch 2 times, most recently from a5e8847 to dbc44a0 Compare June 23, 2025 06:48
# shellcheck disable=SC1090,SC1091
. "$TOOLS/functestlib.sh"

TESTNAME="systemd"
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to change name of the test. As it is doing for process check here.


# Call the functions
check_systemd_pid
check_systemctl_stop
Copy link
Contributor

Choose a reason for hiding this comment

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

Better move services check to different script. Any reason to club together?


ANY_SUBTEST_FAILED="false"

update_test_pass(){
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be moved to common functions

Copy link
Contributor

@smuppand smuppand left a comment

Choose a reason for hiding this comment

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

Require some more changes.

@avisinone
Copy link
Author

Require some more changes.

Updated with suggested changes

check_systemctl_stop() {
log_info "----------------------------------------------------"
log_info "-------- Starting $TESTNAME Functional Test --------"
systemctl stop systemd-user-sessions.service
Copy link
Contributor

Choose a reason for hiding this comment

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

before stopping systemctl, can we ensure it is in active state

log_pass "Able to stop the service systemd-user-sessions with systemctl"
echo "$TESTNAME PASS" >> "$res_file"
fi
log_info "----------------------------------------------------"
Copy link
Contributor

Choose a reason for hiding this comment

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

Also is it required to add cleanup where in you restart the systemctl so that other tests wont get affected?

Copy link
Author

Choose a reason for hiding this comment

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

changed the test for systemctl start stop into one test to not have redundancy

@vnarapar
Copy link
Contributor

vnarapar commented Jul 2, 2025

Can you please add readme aswell

failed_services=$(systemctl --failed --no-legend --plain | awk '{print $1}')
if [ -z "$failed_services" ]; then
log_pass "No service is in failed state on device"
echo "$TESTNAME PASS" >> "$res_file"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please give > instead of >> as this can append the results if new build is not flashed

log_fail "------ List of failed services --------"
log_fail "$failed_services"
log_fail "--------------------------------------"
echo "$TESTNAME FAIL" >> "$res_file"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please give > instead of >> as this can append the results if new build is not flashed

log_info "-------- Stopping systemd-user-sessions.service --------"
if ! systemctl is-active --quiet systemd-user-sessions.service; then
log_info "Service is not active before proceeding with stop command"
echo "$TESTNAME Fail" >> "$res_file"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please give > instead of >> as this can append the results if new build is not flashed

Copy link
Contributor

Choose a reason for hiding this comment

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

Same applies to others aswell

Copy link
Author

Choose a reason for hiding this comment

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

@smuppand Can you please confirm should i change it back to overwriting the result file?

@@ -0,0 +1,27 @@
# systemd Suite
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add readme for each test for consistency

@avisinone avisinone force-pushed the platform_test branch 2 times, most recently from 4cdeae7 to 33900ac Compare July 14, 2025 13:46
Following check are implemented :
systemdPID\run.sh Check systemd starts with PID 1.
systemctlStartStop\run.sh Check if systemctl is able to start and stop a service.
CheckFailedServices\run.sh Check for failed service on device using systemctl status.
Usage Command Verified :
./run-test.sh systemPID
./run-test.sh systemctlStartStop
./run-test.sh CheckFailedServices

Signed-off-by: Abhishek Sinha <[email protected]>
Copy link
Contributor

@vnarapar vnarapar left a comment

Choose a reason for hiding this comment

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

LGTM

@avisinone avisinone requested a review from smuppand July 15, 2025 09:53
Copy link

This pull request has been marked as stale due to 30 days of inactivity. To prevent automatic closure in 7 days, remove the stale label or add a comment. You can reopen a closed pull request at any time.

@github-actions github-actions bot added the Stale label Aug 15, 2025
@github-actions github-actions bot closed this Aug 23, 2025
@vnarapar vnarapar reopened this Sep 19, 2025
@smuppand
Copy link
Contributor

smuppand commented Sep 19, 2025

@avisinone Can you review the errors, resolve them, and upload an updated patch? Please include a more detailed PR description, explaining why this test is necessary and outlining what it will cover.

@github-actions github-actions bot removed the Stale label Sep 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants