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.

Sign up for GitHub

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

Jump to bottom

Fix wpa_supplicant test #12580

Merged
merged 1 commit into from May 25, 2021
Merged

Fix wpa_supplicant test #12580

merged 1 commit into from May 25, 2021

Conversation

DeepthiYV
Copy link
Contributor

@DeepthiYV DeepthiYV commented May 21, 2021 •
edited

AppArmor prevents the hostpad config file from being read. so execution of aa-disable hostapd before starting hostapd.

@DeepthiYV DeepthiYV changed the title Fix wpa_supplicant test [WIP] Fix wpa_supplicant test May 21, 2021
@DeepthiYV DeepthiYV changed the title [WIP] Fix wpa_supplicant test Fix wpa_supplicant test May 21, 2021
ge0r
ge0r reviewed May 21, 2021
View reviewed changes
data/wpa_supplicant/wpa_supplicant_test.sh Outdated
@@ -14,6 +14,7 @@ function cleanup() {
kill $hostapd_pid
fi
modprobe -r mac80211_hwsim
cat hostapd.log
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this for the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, I have just added line to get the results after the hostapd run.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just as suggestion: One could think about using upload_logs in the post-run and post-fail hooks to upload this log as an openQA asset. See e.g. docker if you feel motivated 🙂

@pdostal
Copy link
Member

pdostal commented May 21, 2021

Hello @cfconrad, can you provide more insight in the history of this test?
I think supplicant_test.sh would be very nice candidate to refactor into perl.

grisu48
grisu48 approved these changes May 25, 2021
View reviewed changes
Copy link
Contributor

@grisu48 grisu48 left a comment

Choose a reason for hiding this comment

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

LGTM. I'd still add a SLES verification run (VR) as well, then it's ready to rock.
Congrats 🥳

data/wpa_supplicant/wpa_supplicant_test.sh Outdated
@@ -14,6 +14,7 @@ function cleanup() {
kill $hostapd_pid
fi
modprobe -r mac80211_hwsim
cat hostapd.log
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as suggestion: One could think about using upload_logs in the post-run and post-fail hooks to upload this log as an openQA asset. See e.g. docker if you feel motivated 🙂

cfconrad
cfconrad requested changes May 25, 2021
View reviewed changes
Copy link
Contributor

@cfconrad cfconrad left a comment

Choose a reason for hiding this comment

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

I'm for changing the aa profile or adding allways_rollback as aa is a main feature and I don't like to just disable it for the hole later coming test modules.

tests/console/wpa_supplicant.pm Outdated Show resolved Hide resolved
@cfconrad
Copy link
Contributor

Hello @cfconrad, can you provide more insight in the history of this test?
I think supplicant_test.sh would be very nice candidate to refactor into perl.

It was invented by @grisu48, thx for your research and the really great idea!

Based on that idea there are several tests around it:

I would vote for a perl module which do the common works. And that is actually what lib/wicked/wlan.pm does. Currently we do not run wicked/wlan on Tumbleweed, that is why we didn't caught the aa issue.

@ge0r
Copy link
Member

ge0r commented May 25, 2021

LGTM. I'd still add a SLES verification run (VR) as well, then it's ready to rock.
Congrats partying_face

I agree on the SLES VR. Dee maybe you could run eg this job as a verification run, just to double check that the wpa_supplicant module passes ok on SLES, and that disabling the hostapd apparmor profile does not really change the outcome of modules later in the schedule as cfconrad has hypothesized.

ge0r
ge0r approved these changes May 25, 2021
View reviewed changes
Copy link
Member

@ge0r ge0r left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work, the addressed issues have been resolved!

@ge0r ge0r merged commit a591771 into os-autoinst:master May 25, 2021
cfconrad
cfconrad reviewed May 26, 2021
View reviewed changes
Copy link
Contributor

@cfconrad cfconrad left a comment •
edited

Choose a reason for hiding this comment

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

Thx for the update!
LGTM

@pdostal
Copy link
Member

pdostal commented May 27, 2021

I see many suggestion still opened but not resolved. Why was this merged?

@ge0r
Copy link
Member

ge0r commented May 27, 2021

I see many suggestion still opened but not resolved. Why was this merged?

Well, the suggestions (apart from the idea to move the script to perl) were implemented. I just could not mark them as resolved myself, but I left a comment to indicate it

@cfconrad
Copy link
Contributor

maybe my fault then... but usually if I changed something in the way someone suggested it, I click the resolve button. Especially if someone put a change-required. He will look at it anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pdostal pdostal pdostal left review comments

@grisu48 grisu48 grisu48 approved these changes

@cfconrad cfconrad cfconrad requested changes

@ge0r ge0r ge0r approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@DeepthiYV @pdostal @cfconrad @ge0r @grisu48