Skip to content

Conversation

@1awesomeJ
Copy link
Contributor

No description provided.

@1awesomeJ
Copy link
Contributor Author

Most of the part missing coverage on the cryptenroll.c module are in the parse_argv() function. There are about 300 lines currently not covered.
I've added these tests to cover the first 100.
I'd like some initial reviews while I work on the remaining 200.

Thank you sirs.

@1awesomeJ
Copy link
Contributor Author

For a test like "systemd-cryptenroll --password",
The script prompts the user for the current password, the new password, and a confirmation of the new password.

As it is the script probably doesn't move beyond that test. I'm currently working on how I could pass in those 3 required inputs in the test script, so that the interpreter takes them automatically, executes the test and moves to the next test.

Any insight will be greatly appreciated.

Thank you.

@bluca
Copy link
Member

bluca commented Mar 28, 2023

please rebase on latest main, the test should pass then

@bluca bluca added needs-rebase and removed please-review PR is ready for (re-)review by a maintainer labels Mar 28, 2023
@1awesomeJ
Copy link
Contributor Author

please rebase on latest main, the test should pass then

Exactly sir.

I'll make a push now.

I'll make more push today.

I hope to cover all by tomorrow.

For test cases where a user is supposed to input their credentials sir. What would you recommend I do.

Like "systemd-cryptenroll --password"
This will prompt for passwords.

All I saw while searching was to use expect scripts, which isn't at all used in the codebase.

Thank you sir.

@bluca
Copy link
Member

bluca commented Mar 28, 2023

Set the NEWPASSWORD and PASSWORD environment variables

@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed needs-rebase labels Mar 28, 2023
@1awesomeJ
Copy link
Contributor Author

Set the NEWPASSWORD and PASSWORD environment variables

Thank you so much sir.

@1awesomeJ 1awesomeJ force-pushed the nit branch 3 times, most recently from 86d438e to d3ac85e Compare March 28, 2023 18:19
@1awesomeJ
Copy link
Contributor Author

I did 22 tests total.

For my long task "fix an issue with the label RFE", I will try to work on the cryptenroll tool still.
When I see the updated Coveralls report on the module, I will add any more needed test as a commit for my next PR.
Thank you sir.

@1awesomeJ
Copy link
Contributor Author

1awesomeJ commented Mar 29, 2023

@bluca, @keszybz @mrc0mmand @DaanDeMeyer @yuwata

could you please help confirm if the failure on "mkosi / ci (arch, rolling) (pull request)" is due to my code?

I had "mkos/ ci (centos, 9) (pull request)" as the one failing initially, I wanted to confirm it was due to any issues with my tests,
so I just edited a comment, and made another push to restart the checks, now the "centos 9" passes, and it's "arch, rolling" that fails. "arch, rolling" passed before.

Thank you.

@1awesomeJ
Copy link
Contributor Author

All the usual checks now pass. Thank you sirs.
kindly help review.

@bluca bluca merged commit 89c632d into systemd:main Mar 29, 2023
@github-actions github-actions bot removed the please-review PR is ready for (re-)review by a maintainer label Mar 29, 2023
@1awesomeJ
Copy link
Contributor Author

Awesome. tpm2 pcr RFE next.

Thank you.

Comment on lines +229 to +230
ret="$(! systemd-cryptenroll --fido2-with-client-pin=false 2> >(grep "No block device node specified"))"
test -n "${ret}"
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, what exactly is this test testing? Why not just use a pipe?
(IIUC, the goal is to assert something about stderr, but since stdout and stderr are mixed, it's not really testing much.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw the line for that isn't covered on Coveralls..
So I decided to test such error situation.

Could you clarify further about the pipe sir?

Thank you

Copy link
Member

Choose a reason for hiding this comment

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

What he means is that checking the output like that is not very useful, as we change debug log quite often. Instead, simply check that i's failing as expected, eg:

systemd-cryptenroll --fido2-with-client-pin=false && { echo 'unexpected success'; exit 1; }

please send a follow-up to apply thorough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh wow. Thank you so much sir.
I'm really grateful.
I had this concern of the set -ex flag.
I didn't want the test to exit after just one failed test, as that would make the negative test cases sort of hard to do.

Can I open a new PR on this right now?

Or is there a way to rework this very one?

Copy link
Member

Choose a reason for hiding this comment

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

open a new pr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome.
Thank you sir

@1awesomeJ
Copy link
Contributor Author

new PR opened sir:
#27079

1awesomeJ added a commit to 1awesomeJ/systemd that referenced this pull request Mar 31, 2023
1awesomeJ added a commit to 1awesomeJ/systemd that referenced this pull request Mar 31, 2023
1awesomeJ added a commit to 1awesomeJ/systemd that referenced this pull request Mar 31, 2023
Use in-line error handling rather than redirections. Follow up on systemd#27020
bluca pushed a commit that referenced this pull request Mar 31, 2023
Use in-line error handling rather than redirections. Follow up on #27020
taniishkaaa pushed a commit to taniishkaaa/systemd that referenced this pull request Apr 4, 2023
Use in-line error handling rather than redirections. Follow up on systemd#27020
bluca pushed a commit to bluca/systemd that referenced this pull request Apr 26, 2023
Use in-line error handling rather than redirections. Follow up on systemd#27020

(cherry picked from commit 27d45db)
bluca pushed a commit to bluca/systemd that referenced this pull request Jun 2, 2023
Use in-line error handling rather than redirections. Follow up on systemd#27020

(cherry picked from commit 27d45db)
(cherry picked from commit 2950b4e)
peckato1 pushed a commit to peckato1/systemd that referenced this pull request Jun 12, 2023
Use in-line error handling rather than redirections. Follow up on systemd#27020

(cherry picked from commit 27d45db)
esposem pushed a commit to esposem/systemd that referenced this pull request Aug 29, 2024
Use in-line error handling rather than redirections. Follow up on systemd#27020

(cherry picked from commit 27d45db)

Related: RHEL-16182
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants