Repository: quic-go/quic-go
Pull request #4837 (2025-01-06 07:38:01)

migrate the crypto stream tests away from Ginkgo
Part of #3652.


Patch:
diff --git a/connection.go b/connection.go
index b891b1b1..583e9a90 100644
--- a/connection.go
+++ b/connection.go
@@ -1342,6 +1342,7 @@ func (s *connection) handleFrame(
 		s.handleMaxStreamsFrame(frame)
 	case *wire.DataBlockedFrame:
 	case *wire.StreamDataBlockedFrame:
+		err = s.handleStreamDataBlockedFrame(frame)
 	case *wire.StreamsBlockedFrame:
 	case *wire.StopSendingFrame:
 		err = s.handleStopSendingFrame(frame)
@@ -1477,6 +1478,13 @@ func (s *connection) handleMaxStreamDataFrame(frame *wire.MaxStreamDataFrame) er
 	return nil
 }
 
+func (s *connection) handleStreamDataBlockedFrame(frame *wire.StreamDataBlockedFrame) error {
+	// We don't need to do anything in response to a STREAM_DATA_BLOCKED frame,
+	// but we need to make sure that the stream ID is valid.
+	_, err := s.streamsMap.GetOrOpenReceiveStream(frame.StreamID)
+	return err
+}
+
 func (s *connection) handleMaxStreamsFrame(frame *wire.MaxStreamsFrame) {
 	s.streamsMap.HandleMaxStreamsFrame(frame)
 }
@@ -2021,6 +2029,7 @@ func (s *connection) sendPacketsWithGSO(now time.Time) error {
 			return nil
 		}
 
+		ecn = nextECN
 		buf = getLargePacketBuffer()
 	}
 }


Test patch:
diff --git a/connection_test.go b/connection_test.go
index 47b9e1ca..695fb7c6 100644
--- a/connection_test.go
+++ b/connection_test.go
@@ -223,6 +223,7 @@ func TestConnectionHandleReceiveStreamFrames(t *testing.T) {
 		str.EXPECT().handleResetStreamFrame(rsf, now)
 		require.NoError(t, tc.conn.handleFrame(rsf, protocol.Encryption1RTT, connID, now))
 		// STREAM_DATA_BLOCKED frames are not passed to the stream
+		streamsMap.EXPECT().GetOrOpenReceiveStream(streamID).Return(str, nil)
 		require.NoError(t, tc.conn.handleFrame(sdbf, protocol.Encryption1RTT, connID, now))
 	})
 
@@ -237,7 +238,7 @@ func TestConnectionHandleReceiveStreamFrames(t *testing.T) {
 		streamsMap.EXPECT().GetOrOpenReceiveStream(streamID).Return(nil, nil)
 		require.NoError(t, tc.conn.handleFrame(rsf, protocol.Encryption1RTT, connID, now))
 		// STREAM_DATA_BLOCKED frames are not passed to the stream
-		// TODO(#4822): validate stream ID of STREAM_DATA_BLOCKED frames
+		streamsMap.EXPECT().GetOrOpenReceiveStream(streamID).Return(nil, nil)
 		require.NoError(t, tc.conn.handleFrame(sdbf, protocol.Encryption1RTT, connID, now))
 	})
 
@@ -253,8 +254,8 @@ func TestConnectionHandleReceiveStreamFrames(t *testing.T) {
 		streamsMap.EXPECT().GetOrOpenReceiveStream(streamID).Return(nil, testErr)
 		require.ErrorIs(t, tc.conn.handleFrame(rsf, protocol.Encryption1RTT, connID, now), testErr)
 		// STREAM_DATA_BLOCKED frames are not passed to the stream
-		// TODO(#4822): validate stream ID of STREAM_DATA_BLOCKED frames
-		require.NoError(t, tc.conn.handleFrame(sdbf, protocol.Encryption1RTT, connID, now))
+		streamsMap.EXPECT().GetOrOpenReceiveStream(streamID).Return(nil, testErr)
+		require.ErrorIs(t, tc.conn.handleFrame(sdbf, protocol.Encryption1RTT, connID, now), testErr)
 	})
 }
 
@@ -2113,8 +2114,7 @@ func TestConnectionGSOBatchECN(t *testing.T) {
 
 	done3 := make(chan struct{})
 	tc.sendConn.EXPECT().Write(expectedData, uint16(maxPacketSize), protocol.ECT1)
-	// TODO(#4829): check that the correct ECN marking is used
-	tc.sendConn.EXPECT().Write([]byte("foobar"), uint16(maxPacketSize), gomock.Any()).DoAndReturn(
+	tc.sendConn.EXPECT().Write([]byte("foobar"), uint16(maxPacketSize), protocol.ECNCE).DoAndReturn(
 		func([]byte, uint16, protocol.ECN) error { close(done3); return nil },
 	)
 
diff --git a/crypto_stream_test.go b/crypto_stream_test.go
index 7a5eb9df..6b548882 100644
--- a/crypto_stream_test.go
+++ b/crypto_stream_test.go
@@ -1,158 +1,101 @@
 package quic
 
 import (
-	"fmt"
+	"testing"
 
 	"github.com/quic-go/quic-go/internal/protocol"
 	"github.com/quic-go/quic-go/internal/qerr"
 	"github.com/quic-go/quic-go/internal/wire"
 
-	. "github.com/onsi/ginkgo/v2"
-	. "github.com/onsi/gomega"
+	"github.com/stretchr/testify/require"
 )
 
-var _ = Describe("Crypto Stream", func() {
-	var str *cryptoStream
-
-	BeforeEach(func() { str = newCryptoStream() })
-
-	Context("handling incoming data", func() {
-		It("handles in-order CRYPTO frames", func() {
-			Expect(str.HandleCryptoFrame(&wire.CryptoFrame{Data: []byte("foo")})).To(Succeed())
-			Expect(str.GetCryptoData()).To(Equal([]byte("foo")))
-			Expect(str.GetCryptoData()).To(BeNil())
-			Expect(str.HandleCryptoFrame(&wire.CryptoFrame{Data: []byte("bar"), Offset: 3})).To(Succeed())
-			Expect(str.GetCryptoData()).To(Equal([]byte("bar")))
-			Expect(str.GetCryptoData()).To(BeNil())
-		})
-
-		It("errors if the frame exceeds the maximum offset", func() {
-			Expect(str.HandleCryptoFrame(&wire.CryptoFrame{
-				Offset: protocol.MaxCryptoStreamOffset - 5,
-				Data:   []byte("foobar"),
-			})).To(MatchError(&qerr.TransportError{
-				ErrorCode:    qerr.CryptoBufferExceeded,
-				ErrorMessage: fmt.Sprintf("received invalid offset %d on crypto stream, maximum allowed %d", protocol.MaxCryptoStreamOffset+1, protocol.MaxCryptoStreamOffset),
-			}))
-		})
-
-		It("handles out-of-order CRYPTO frames", func() {
-			Expect(str.HandleCryptoFrame(&wire.CryptoFrame{Offset: 3, Data: []byte("bar")})).To(Succeed())
-			Expect(str.HandleCryptoFrame(&wire.CryptoFrame{Data: []byte("foo")})).To(Succeed())
-			var data []byte
-			for {
-				b := str.GetCryptoData()
-				if b == nil {
-					break
-				}
-				data = append(data, b...)
-			}
-			Expect(data).To(Equal([]byte("foobar")))
-			Expect(str.GetCryptoData()).To(BeNil())
-		})
-
-		Context("finishing", func() {
-			It("errors if there's still data to read at the current offset after finishing", func() {
-				Expect(str.HandleCryptoFrame(&wire.CryptoFrame{
-					Data: []byte("foo"),
-				})).To(Succeed())
-				Expect(str.Finish()).To(MatchError(&qerr.TransportError{
-					ErrorCode:    qerr.ProtocolViolation,
-					ErrorMessage: "encryption level changed, but crypto stream has more data to read",
-				}))
-			})
-
-			It("errors if there's still data to read at a higher offset after finishing", func() {
-				Expect(str.HandleCryptoFrame(&wire.CryptoFrame{
-					Data:   []byte("foobar"),
-					Offset: 10,
-				})).To(Succeed())
-				Expect(str.Finish()).To(MatchError(&qerr.TransportError{
-					ErrorCode:    qerr.ProtocolViolation,
-					ErrorMessage: "encryption level changed, but crypto stream has more data to read",
-				}))
-			})
-
-			It("works with reordered data", func() {
-				f1 := &wire.CryptoFrame{
-					Data: []byte("foo"),
-				}
-				f2 := &wire.CryptoFrame{
-					Offset: 3,
-					Data:   []byte("bar"),
-				}
-				Expect(str.HandleCryptoFrame(f2)).To(Succeed())
-				Expect(str.HandleCryptoFrame(f1)).To(Succeed())
-				Expect(str.GetCryptoData()).To(HaveLen(3))
-				Expect(str.GetCryptoData()).To(HaveLen(3))
-				Expect(str.Finish()).To(Succeed())
-				Expect(str.HandleCryptoFrame(f2)).To(Succeed())
-			})
-
-			It("rejects new crypto data after finishing", func() {
-				Expect(str.Finish()).To(Succeed())
-				Expect(str.HandleCryptoFrame(&wire.CryptoFrame{
-					Data: []byte("foo"),
-				})).To(MatchError(&qerr.TransportError{
-					ErrorCode:    qerr.ProtocolViolation,
-					ErrorMessage: "received crypto data after change of encryption level",
-				}))
-			})
-
-			It("ignores crypto data below the maximum offset received before finishing", func() {
-				Expect(str.HandleCryptoFrame(&wire.CryptoFrame{
-					Data: []byte("foobar"),
-				})).To(Succeed())
-				Expect(str.GetCryptoData()).To(Equal([]byte("foobar")))
-				Expect(str.Finish()).To(Succeed())
-				Expect(str.HandleCryptoFrame(&wire.CryptoFrame{
-					Offset: 2,
-					Data:   []byte("foo"),
-				})).To(Succeed())
-			})
-		})
+func TestCryptoStreamDataAssembly(t *testing.T) {
+	str := newCryptoStream()
+	require.NoError(t, str.HandleCryptoFrame(&wire.CryptoFrame{Data: []byte("bar"), Offset: 3}))
+	require.NoError(t, str.HandleCryptoFrame(&wire.CryptoFrame{Data: []byte("foo")}))
+	// receive a retransmission
+	require.NoError(t, str.HandleCryptoFrame(&wire.CryptoFrame{Data: []byte("bar"), Offset: 3}))
+
+	var data []byte
+	for {
+		b := str.GetCryptoData()
+		if b == nil {
+			break
+		}
+		data = append(data, b...)
+	}
+	require.Equal(t, []byte("foobar"), data)
+}
+
+func TestCryptoStreamMaxOffset(t *testing.T) {
+	str := newCryptoStream()
+	require.NoError(t, str.HandleCryptoFrame(&wire.CryptoFrame{
+		Offset: protocol.MaxCryptoStreamOffset - 5,
+		Data:   []byte("foo"),
+	}))
+	require.ErrorIs(t,
+		str.HandleCryptoFrame(&wire.CryptoFrame{
+			Offset: protocol.MaxCryptoStreamOffset - 2,
+			Data:   []byte("bar"),
+		}),
+		&qerr.TransportError{ErrorCode: qerr.CryptoBufferExceeded},
+	)
+}
+
+func TestCryptoStreamFinishWithQueuedData(t *testing.T) {
+	t.Run("with data at current offset", func(t *testing.T) {
+		str := newCryptoStream()
+		require.NoError(t, str.HandleCryptoFrame(&wire.CryptoFrame{Data: []byte("foo")}))
+		require.Equal(t, []byte("foo"), str.GetCryptoData())
+		require.NoError(t, str.HandleCryptoFrame(&wire.CryptoFrame{Data: []byte("bar"), Offset: 3}))
+		require.ErrorIs(t, str.Finish(), &qerr.TransportError{ErrorCode: qerr.ProtocolViolation})
 	})
 
-	Context("writing data", func() {
-		It("says if it has data", func() {
-			Expect(str.HasData()).To(BeFalse())
-			_, err := str.Write([]byte("foobar"))
-			Expect(err).ToNot(HaveOccurred())
-			Expect(str.HasData()).To(BeTrue())
-		})
-
-		It("pops crypto frames", func() {
-			_, err := str.Write([]byte("foobar"))
-			Expect(err).ToNot(HaveOccurred())
-			f := str.PopCryptoFrame(1000)
-			Expect(f).ToNot(BeNil())
-			Expect(f.Offset).To(BeZero())
-			Expect(f.Data).To(Equal([]byte("foobar")))
-		})
-
-		It("coalesces multiple writes", func() {
-			_, err := str.Write([]byte("foo"))
-			Expect(err).ToNot(HaveOccurred())
-			_, err = str.Write([]byte("bar"))
-			Expect(err).ToNot(HaveOccurred())
-			f := str.PopCryptoFrame(1000)
-			Expect(f).ToNot(BeNil())
-			Expect(f.Offset).To(BeZero())
-			Expect(f.Data).To(Equal([]byte("foobar")))
-		})
-
-		It("respects the maximum size", func() {
-			frameHeaderLen := (&wire.CryptoFrame{}).Length(protocol.Version1)
-			_, err := str.Write([]byte("foobar"))
-			Expect(err).ToNot(HaveOccurred())
-			f := str.PopCryptoFrame(frameHeaderLen + 3)
-			Expect(f).ToNot(BeNil())
-			Expect(f.Offset).To(BeZero())
-			Expect(f.Data).To(Equal([]byte("foo")))
-			f = str.PopCryptoFrame(frameHeaderLen + 3)
-			Expect(f).ToNot(BeNil())
-			Expect(f.Offset).To(Equal(protocol.ByteCount(3)))
-			Expect(f.Data).To(Equal([]byte("bar")))
-		})
+	t.Run("with data at a higher offset", func(t *testing.T) {
+		str := newCryptoStream()
+		require.NoError(t, str.HandleCryptoFrame(&wire.CryptoFrame{Data: []byte("foobar"), Offset: 20}))
+		require.ErrorIs(t, str.Finish(), &qerr.TransportError{ErrorCode: qerr.ProtocolViolation})
 	})
-})
+}
+
+func TestCryptoStreamReceiveDataAfterFinish(t *testing.T) {
+	str := newCryptoStream()
+	require.NoError(t, str.HandleCryptoFrame(&wire.CryptoFrame{Data: []byte("foobar")}))
+	require.Equal(t, []byte("foobar"), str.GetCryptoData())
+	require.NoError(t, str.Finish())
+	// receiving a retransmission is ok
+	require.NoError(t, str.HandleCryptoFrame(&wire.CryptoFrame{Data: []byte("bar"), Offset: 3}))
+	// but receiving new data is not
+	require.ErrorIs(t,
+		str.HandleCryptoFrame(&wire.CryptoFrame{Data: []byte("baz"), Offset: 4}),
+		&qerr.TransportError{ErrorCode: qerr.ProtocolViolation},
+	)
+}
+
+func TestCryptoStreamWrite(t *testing.T) {
+	expectedCryptoFrameLen := func(offset protocol.ByteCount) protocol.ByteCount {
+		f := &wire.CryptoFrame{Offset: offset}
+		return f.Length(protocol.Version1)
+	}
+
+	str := newCryptoStream()
+
+	require.False(t, str.HasData())
+	_, err := str.Write([]byte("foo"))
+	require.NoError(t, err)
+	require.True(t, str.HasData())
+	_, err = str.Write([]byte("bar"))
+	require.NoError(t, err)
+	_, err = str.Write([]byte("baz"))
+	require.NoError(t, err)
+	require.True(t, str.HasData())
+
+	f := str.PopCryptoFrame(expectedCryptoFrameLen(0) + 3)
+	require.Equal(t, &wire.CryptoFrame{Data: []byte("foo")}, f)
+	require.True(t, str.HasData())
+	f = str.PopCryptoFrame(protocol.MaxByteCount)
+	// the two write calls were coalesced into a single frame
+	require.Equal(t, &wire.CryptoFrame{Offset: 3, Data: []byte("barbaz")}, f)
+	require.False(t, str.HasData())
+}