Skip to content

Refactor CLI to make it testable using testscript #1426

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
Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Refactor CLI to make it testable using testscript #1426

wants to merge 9 commits into from
+1,953 −1,947

Conversation

hslatman
Copy link
Member

@hslatman hslatman commented May 22, 2025 •
edited
Loading

This PR refactors the CLI and its integration tests to use the testscript package.

As part of that, this PR:

  • Refactors the existing integration tests to use the testscript DSL
  • Removes the integration build tag
  • Removes any notion of running integration tests; they're part of the usual test suite now

@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label May 22, 2025
@hslatman hslatman force-pushed the herman/testscript branch from 4fc2791 to b69ee0e Compare May 22, 2025 10:33
@hslatman hslatman added this to the v0.28.7 milestone May 26, 2025
This was referenced May 27, 2025
Closed
Merged
@hslatman hslatman force-pushed the herman/testscript branch 3 times, most recently from ae24576 to 4832304 Compare May 30, 2025 17:27
hslatman added 4 commits June 12, 2025 15:24
@hslatman
Refactor CLI to make it testable using testscript
f338d80
@hslatman
Replace several old integration style tests with testscript tests
825b161
@hslatman
Refactor step crypto keypair tests to use testscript
1fc663b
@hslatman
Use --password-file to provide password to step crypto keypair
f641332 
In CI the TTY is not available, and will result in this error:

`error reading password: error allocating terminal: open /dev/tty: no such device or address`

We prevent this by using the `--password-file` flag.
@hslatman hslatman force-pushed the herman/testscript branch from 4832304 to 6de4059 Compare June 12, 2025 13:24
@hslatman hslatman force-pushed the herman/testscript branch from 6de4059 to 298ff27 Compare June 12, 2025 13:31
@hslatman hslatman force-pushed the herman/testscript branch from f229d6a to 1a494ce Compare June 12, 2025 23:00
@hslatman hslatman marked this pull request as ready for review June 13, 2025 10:40
@hslatman hslatman requested review from a team and maraino June 13, 2025 10:42
@hslatman hslatman force-pushed the herman/testscript branch from eb550ee to 92a5eab Compare June 13, 2025 13:28
@hslatman hslatman force-pushed the herman/testscript branch from 92a5eab to 2944159 Compare June 13, 2025 13:33
maraino
maraino requested changes Jun 16, 2025
View reviewed changes
Copy link
Collaborator

@maraino maraino left a comment

Choose a reason for hiding this comment

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

A few comments, and a rename of a couple of files.

internal/cmd/main.go Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably rename this file and main_test.go to something different. Cobra usually names this file root.go.

Copy link
Member Author

Choose a reason for hiding this comment

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

1932842

internal/cmd/main.go Outdated
Comment on lines 106 to 107
app.Name = "step"
app.HelpName = "step"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although this was like this before, in some OSs the step binary is distributed as step-cli, can this be a problem here? We could allow to inject the value using -X ldflags, using step by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if it can be a problem, but it can now be set with 279e638. Using the same name for both seemed sufficient to me.

integration/certificate_test.go
Comment on lines +21 to +40
signer, err := keyutil.GenerateDefaultSigner()
require.NoError(t, err)
csrBytes, err := x509.CreateCertificateRequest(rand.Reader, &x509.CertificateRequest{Subject: pkix.Name{CommonName: "test"}}, signer)
require.NoError(t, err)
csr, err := x509.ParseCertificateRequest(csrBytes)
require.NoError(t, err)
caSigner, err := keyutil.GenerateDefaultSigner()
require.NoError(t, err)
tmpl := &x509.Certificate{
Subject: pkix.Name{CommonName: "test-ca"},
SerialNumber: big.NewInt(1),
IsCA: true,
MaxPathLen: 1,
BasicConstraintsValid: true,
KeyUsage: x509.KeyUsageCertSign,
}
caCertBytes, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, caSigner.Public(), caSigner)
require.NoError(t, err)
caCert, err := x509.ParseCertificate(caCertBytes)
require.NoError(t, err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use minica to make this shorter.

Copy link
Member Author

Choose a reason for hiding this comment

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

minica didn't give me access to the caSigner key, which this test needs to write out to disk

integration/testdata/certificate/sign.txtar Outdated
@@ -0,0 +1,2 @@
exec step certificate sign test.csr cacert.pem cakey.pem
stdout '-----BEGIN CERTIFICATE-----'
Copy link
Collaborator

Choose a reason for hiding this comment

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

As an improvement a custom checker can also be implemented.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, great suggestion 🙂: 1932842

integration/testdata/crypto/jwk-create-ec.txtar Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can add tests using --no-password --insecure, also making sure that --no-password required --insecure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The same can be applied to other files.

Copy link
Member Author

Choose a reason for hiding this comment

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

1932842

integration/testdata/crypto/jwt-sign.txtar
# P-256 sign with subtle flag
exec step crypto jwt sign -key p256.pem -iss TestIssuer -aud TestAudience -sub TestSubject -nbf 1 -iat 1 -exp 1 -subtle
stdout 'eyJhbGciOiJFUzI1NiIsImtpZCI6Ii1pZ1pNalRCdkhFRG02bjkxQkgwT0k4ZUhqQko2b0I3UlpIZFA0RE81U0EiLCJ0eXAiOiJKV1QifQ'

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be consistent, I think we can remove an extra new line here

Suggested change

Copy link
Member Author

Choose a reason for hiding this comment

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

1932842

@hslatman hslatman requested a review from maraino June 17, 2025 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maraino maraino Awaiting requested review from maraino

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
needs triage Waiting for discussion / prioritization by team
Projects
None yet
Milestone
v0.28.7
Development

Successfully merging this pull request may close these issues.
2 participants
@hslatman @maraino