[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6410: TPv2: Improve performance of hostname regular expression

[GitBox <gi...@apache.org>]

ocket8888 commented on a change in pull request #6410:
URL: https://github.com/apache/trafficcontrol/pull/6410#discussion_r764503874



##########
File path: experimental/traffic-portal/src/app/core/new-delivery-service/new-delivery-service.component.spec.ts
##########
@@ -137,4 +137,29 @@ describe("NewDeliveryServiceComponent", () => {
 	// 	component.cdnObject.setValue({ name: 'testCDN', id: 1 } as CDN);
 	// 	component.dsType.setValue({ name: 'testType', id: 1 } as Type);
 	// });
+
+	it("should match domain names", async () => {
+		const invalidDomains: Array<string> = [
+			"d.",
+			"d-",
+			"d-.o",
+			"-d.o",
+		];
+		for (const invalidDomain of invalidDomains) {
+			expect(() => {
+				component.setDNSBypass(invalidDomain);
+			}).toThrow();
+		}
+		const validDomains: Array<string> = [
+			"d",
+			"d.o.m.a.i.n",
+			"d-o-------m.ain",
+		];
+		expect(() => {
+			for (const domain of validDomains) {
+				component.setDNSBypass(domain);
+				expect(component.deliveryService.dnsBypassCname).toBe(domain);
+			}
+		}).not.toThrow();

Review comment:
       These tests _could_ just be passing because the inputs are being mistaken for valid IP(v4 or v6) addresses - would you mind testing for those as well? If it's easier, feel free to just export the regular expression (and in that case no additional test cases would be necessary, though because it's necessarily testing less)

##########
File path: experimental/traffic-portal/src/app/core/new-delivery-service/new-delivery-service.component.spec.ts
##########
@@ -137,4 +137,14 @@ describe("NewDeliveryServiceComponent", () => {
 	// 	component.cdnObject.setValue({ name: 'testCDN', id: 1 } as CDN);
 	// 	component.dsType.setValue({ name: 'testType', id: 1 } as Type);
 	// });
+
+	it("should match hostnames", async () => {
+		const invalidHostnames: Array<string> = ["h.", "h-", "h-.o", "-h.o"];
+		invalidHostnames.forEach((invalidHostname: string) => void expect(() => component.setDNSBypass(invalidHostname)).toThrow());
+		const validHostnames: Array<string> = ["h", "h.o.s.T.n.a.m.e", "h-O-------s.tNaMe"];
+		expect(() => validHostnames.forEach((hostname: string) => {
+			component.setDNSBypass(hostname);
+			expect(component.deliveryService.dnsBypassCname).toBe(hostname);
+		})).not.toThrow();

Review comment:
       These tests could just be passing because the inputs are being mistaken for valid IP(v4 or v6) addresses - would you mind testing for those as well? If it's easier, feel free to just export the regular expression (and in that case no additional test cases would be necessary, though because it's necessarily testing less)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org