Skip to content

Conversation

watal
Copy link
Member

@watal watal commented Jul 23, 2025

Description

Add Go test

Type of change

  • Refactoring

k1yoto and others added 30 commits July 22, 2025 18:03
@watal watal changed the base branch from main to develop July 23, 2025 18:23
@watal watal force-pushed the feature/add_test branch from 35ff2c3 to 89c51cf Compare July 24, 2025 08:48
@watal watal requested a review from Copilot July 24, 2025 08:48
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds Go test coverage for PCEP TLV functionality, refactoring existing tests to use common helper functions and adding comprehensive test cases for various TLV types. The refactoring improves test maintainability and consistency across different TLV implementations.

Key changes include:

  • Introduced common test helper functions for TLV testing (decode, serialize, capability strings, length tests)
  • Added comprehensive test coverage for all major TLV types including StatefulPCECapability, SymbolicPathName, IPv4/IPv6LSPIdentifiers, LSPDBVersion, SRPCECapability, PathSetupType, and ExtendedAssociationID
  • Updated TLV implementation code with bug fixes and improvements, including proper flag handling and serialization methods

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
pkg/packet/pcep/tlv_test.go Comprehensive refactoring of TLV tests with new helper functions and extensive test coverage
pkg/packet/pcep/tlv.go Bug fixes and improvements to TLV implementations including flag handling and serialization
pkg/packet/pcep/pcep_util_test.go Added test for Uint64ToByteSlice utility function
pkg/packet/pcep/pcep_util.go Added Uint64ToByteSlice utility function
pkg/packet/pcep/capability_test.go Added test case for LSPDBVersion capability filtering
go.mod Updated Go version and dependencies
api/pola/v1/pola_grpc.pb.go Minor import formatting change
api/pola/v1/pola.pb.go Minor import formatting change
Comments suppressed due to low confidence (2)

pkg/packet/pcep/tlv_test.go:276

  • The test case 'InvalidUTF8Sequence' expects an error but the DecodeFromBytes method for SymbolicPathName doesn't validate UTF-8. This test may not behave as expected unless UTF-8 validation is implemented in the DecodeFromBytes method.
		"InvalidUTF8Sequence":    {testSymbolicPathNameInvalidUTF8Bytes, nil, true},

pkg/packet/pcep/tlv_test.go:813

  • The test case 'UnsupportedLength' creates an ExtendedAssociationID with only Color set, which may not properly test the unsupported length scenario. Consider creating a test case with an explicitly unsupported endpoint type.
		"UnsupportedLength": {&ExtendedAssociationID{Color: 1}, 0},

// Serialized TLV bytes with an invalid UTF-8 sequence.
testSymbolicPathNameInvalidUTF8Bytes = []byte{
byte(TLVSymbolicPathName >> 8), byte(TLVSymbolicPathName & 0xff),
0x00, 0x01, 0xff,
Copy link
Preview

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

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

The magic number 0xff should be documented or replaced with a named constant to clarify that this represents an invalid UTF-8 sequence.

Suggested change
0x00, 0x01, 0xff,
0x00, 0x01, InvalidUTF8Byte,

Copilot uses AI. Check for mistakes.

Comment on lines +454 to +458
tlv.IPv4TunnelSenderAddress, _ = netip.AddrFromSlice(data[:IPv4LSPIdentifiersLSPIDOffset])
tlv.LSPID = binary.BigEndian.Uint16(data[IPv4LSPIdentifiersLSPIDOffset:IPv4LSPIdentifiersTunnelIDOffset])
tlv.TunnelID = binary.BigEndian.Uint16(data[IPv4LSPIdentifiersTunnelIDOffset:IPv4LSPIdentifiersExtendedTunnelIDOffset])
tlv.ExtendedTunnelID = binary.BigEndian.Uint32(data[IPv4LSPIdentifiersExtendedTunnelIDOffset:IPv4LSPIdentifiersTunnelEndpointAddressOffset])
tlv.IPv4TunnelEndpointAddress, _ = netip.AddrFromSlice(data[IPv4LSPIdentifiersTunnelEndpointAddressOffset:TLVIPv4LSPIdentifiersValueLength])
Copy link
Preview

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

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

Error handling is ignored with the blank identifier. If netip.AddrFromSlice fails, it should return an error rather than silently continuing with an invalid address.

Suggested change
tlv.IPv4TunnelSenderAddress, _ = netip.AddrFromSlice(data[:IPv4LSPIdentifiersLSPIDOffset])
tlv.LSPID = binary.BigEndian.Uint16(data[IPv4LSPIdentifiersLSPIDOffset:IPv4LSPIdentifiersTunnelIDOffset])
tlv.TunnelID = binary.BigEndian.Uint16(data[IPv4LSPIdentifiersTunnelIDOffset:IPv4LSPIdentifiersExtendedTunnelIDOffset])
tlv.ExtendedTunnelID = binary.BigEndian.Uint32(data[IPv4LSPIdentifiersExtendedTunnelIDOffset:IPv4LSPIdentifiersTunnelEndpointAddressOffset])
tlv.IPv4TunnelEndpointAddress, _ = netip.AddrFromSlice(data[IPv4LSPIdentifiersTunnelEndpointAddressOffset:TLVIPv4LSPIdentifiersValueLength])
var err error
tlv.IPv4TunnelSenderAddress, err = netip.AddrFromSlice(data[:IPv4LSPIdentifiersLSPIDOffset])
if err != nil {
return fmt.Errorf("failed to parse IPv4TunnelSenderAddress: %w", err)
}
tlv.LSPID = binary.BigEndian.Uint16(data[IPv4LSPIdentifiersLSPIDOffset:IPv4LSPIdentifiersTunnelIDOffset])
tlv.TunnelID = binary.BigEndian.Uint16(data[IPv4LSPIdentifiersTunnelIDOffset:IPv4LSPIdentifiersExtendedTunnelIDOffset])
tlv.ExtendedTunnelID = binary.BigEndian.Uint32(data[IPv4LSPIdentifiersExtendedTunnelIDOffset:IPv4LSPIdentifiersTunnelEndpointAddressOffset])
tlv.IPv4TunnelEndpointAddress, err = netip.AddrFromSlice(data[IPv4LSPIdentifiersTunnelEndpointAddressOffset:TLVIPv4LSPIdentifiersValueLength])
if err != nil {
return fmt.Errorf("failed to parse IPv4TunnelEndpointAddress: %w", err)
}

Copilot uses AI. Check for mistakes.

Comment on lines +525 to +529
tlv.IPv6TunnelSenderAddress, _ = netip.AddrFromSlice(data[:IPv6LSPIdentifiersLSPIDOffset])
tlv.LSPID = binary.BigEndian.Uint16(data[IPv6LSPIdentifiersLSPIDOffset:IPv6LSPIdentifiersTunnelIDOffset])
tlv.TunnelID = binary.BigEndian.Uint16(data[IPv6LSPIdentifiersTunnelIDOffset:IPv6LSPIdentifiersExtendedTunnelIDOffset])
copy(tlv.ExtendedTunnelID[:], data[IPv6LSPIdentifiersExtendedTunnelIDOffset:IPv6LSPIdentifiersTunnelEndpointAddressOffset])
tlv.IPv6TunnelEndpointAddress, _ = netip.AddrFromSlice(data[IPv6LSPIdentifiersTunnelEndpointAddressOffset:TLVIPv6LSPIdentifiersValueLength])
Copy link
Preview

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

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

Error handling is ignored with the blank identifier. If netip.AddrFromSlice fails, it should return an error rather than silently continuing with an invalid address.

Suggested change
tlv.IPv6TunnelSenderAddress, _ = netip.AddrFromSlice(data[:IPv6LSPIdentifiersLSPIDOffset])
tlv.LSPID = binary.BigEndian.Uint16(data[IPv6LSPIdentifiersLSPIDOffset:IPv6LSPIdentifiersTunnelIDOffset])
tlv.TunnelID = binary.BigEndian.Uint16(data[IPv6LSPIdentifiersTunnelIDOffset:IPv6LSPIdentifiersExtendedTunnelIDOffset])
copy(tlv.ExtendedTunnelID[:], data[IPv6LSPIdentifiersExtendedTunnelIDOffset:IPv6LSPIdentifiersTunnelEndpointAddressOffset])
tlv.IPv6TunnelEndpointAddress, _ = netip.AddrFromSlice(data[IPv6LSPIdentifiersTunnelEndpointAddressOffset:TLVIPv6LSPIdentifiersValueLength])
var err error
tlv.IPv6TunnelSenderAddress, err = netip.AddrFromSlice(data[:IPv6LSPIdentifiersLSPIDOffset])
if err != nil {
return fmt.Errorf("failed to parse IPv6TunnelSenderAddress: %w", err)
}
tlv.LSPID = binary.BigEndian.Uint16(data[IPv6LSPIdentifiersLSPIDOffset:IPv6LSPIdentifiersTunnelIDOffset])
tlv.TunnelID = binary.BigEndian.Uint16(data[IPv6LSPIdentifiersTunnelIDOffset:IPv6LSPIdentifiersExtendedTunnelIDOffset])
copy(tlv.ExtendedTunnelID[:], data[IPv6LSPIdentifiersExtendedTunnelIDOffset:IPv6LSPIdentifiersTunnelEndpointAddressOffset])
tlv.IPv6TunnelEndpointAddress, err = netip.AddrFromSlice(data[IPv6LSPIdentifiersTunnelEndpointAddressOffset:TLVIPv6LSPIdentifiersValueLength])
if err != nil {
return fmt.Errorf("failed to parse IPv6TunnelEndpointAddress: %w", err)
}

Copilot uses AI. Check for mistakes.

}

tlv.Endpoint, _ = netip.AddrFromSlice(addrBytes)
Copy link
Preview

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

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

Error handling is ignored with the blank identifier. If netip.AddrFromSlice fails, it should return an error rather than silently continuing with an invalid endpoint address.

Suggested change
tlv.Endpoint, _ = netip.AddrFromSlice(addrBytes)
var err error
tlv.Endpoint, err = netip.AddrFromSlice(addrBytes)
if err != nil {
return fmt.Errorf("extended association ID: invalid endpoint address: %w", err)
}

Copilot uses AI. Check for mistakes.

@Motok1 Motok1 force-pushed the develop branch 10 times, most recently from d420884 to 961c1c7 Compare September 1, 2025 05:54
@watal watal force-pushed the develop branch 3 times, most recently from 8c61705 to 008c478 Compare September 5, 2025 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants