-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Replace badssl.com in tests #2543
Conversation
#2578 is somewhat connected |
Not really - the problem there really is the fact that the go stdlib doesn't returned typed errors with 1.18 on mac os and apperantly using the system CA roots. Not that the test fail as badssl isn't there or does something wrong. |
8c8ae6c
to
52cb2a9
Compare
More or less all of this is really fragile - changing more or less anything breaks one or more of the tests. Unfortunately we once again have a failing CI due ot badssl renewal problems, so even this more fragile code is better. It also happens to be fairly decoupled from any other tests so it should not fail just because we change something for another test 🤞 . If this is approved I will open an issue to fix this in the future ™️ |
@@ -2439,3 +2530,120 @@ func TestBinaryResponseWithStatus0(t *testing.T) { | |||
`) | |||
require.NoError(t, err) | |||
} | |||
|
|||
func GenerateTLSCertificate(t *testing.T, host string, notBefore time.Time, validFor time.Duration) ([]byte, []byte) { |
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.
hmm can you add these helper functions to the go.k6.io/k6/lib/testutils/httpmultibin
or go.k6.io/k6/lib/testutils
package, then you won't need to copy them in 2 places almost verbatim?
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.
They are different so - no we can't :)
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.
Yeah, but I compared them and the differences seemed minor enough to parameterize? In any case, not a deal breaker, it can be fixed later or not at all - anything is better than relying on badssl 😅
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.
LGTM besides the minor nitpick I mentioned inline. I only skimmed some parts of this PR, so I haven't double-checked if the broken crypto is broken exactly as we want it to 😅 But I don't think it matters all that much, anything is better than having these tests randomly break every few weeks...
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.
👍 great work!
A minor nitpick, maybe is a suggestion to not use domain names like mybadssl.com
or tlsv10.com
, but explicitly name them bad-ssl-stub.localhost
or whatever.
Something that indicates that this is a local/stub/fake domain without digging up.
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.
💪
fixes #2302