From 093d9ef54b88e70152d2d4f018921dba2ce0c8a8 Mon Sep 17 00:00:00 2001 From: Roger Floriano <31597636+petruki@users.noreply.github.com> Date: Sat, 27 May 2023 15:30:43 -0700 Subject: [PATCH] Fixed major code smells (#9) * Fixed major code smells * Added test for extractSegment --- src/lib/cache.ts | 11 ++- src/lib/utils.ts | 19 +++--- src/skimming.ts | 9 +-- test/cache_test.ts | 165 +++++++++++++++++++++++++++++++++++---------- test/mod_test.ts | 99 ++++++++++++++++++++++++++- test/utils_test.ts | 39 +++++++++++ 6 files changed, 286 insertions(+), 56 deletions(-) create mode 100644 test/utils_test.ts diff --git a/src/lib/cache.ts b/src/lib/cache.ts index eca524a..b7cc452 100644 --- a/src/lib/cache.ts +++ b/src/lib/cache.ts @@ -42,10 +42,11 @@ export default class CacheHandler { output.segment = output.segment.filter((segment) => { if (ignoreCase) { return segment.toLowerCase().indexOf(query.toLowerCase()) >= 0; - } else { - return segment.indexOf(query) >= 0; } + + return segment.indexOf(query) >= 0; }); + return output.segment.length; }); @@ -116,9 +117,7 @@ export default class CacheHandler { return this.cache.filter((storedData) => { if (storedData.query.length <= query.length) { if (ignoreCase) { - if (!query.toLowerCase().startsWith(storedData.query.toLowerCase())) { - return false; - } + return query.toLowerCase().startsWith(storedData.query.toLowerCase()); } if (query.startsWith(storedData.query) && storedData.exp > Date.now()) { @@ -134,7 +133,7 @@ export default class CacheHandler { } /** - * Verifies if options has been changed, if so it will gather the content from the source again + * Verifies if options has been changed, if so it will fetch the content from the source again * * @param storedData * @param FetchOptions diff --git a/src/lib/utils.ts b/src/lib/utils.ts index 2ed911c..c13dfd4 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -22,7 +22,7 @@ export function validateQuery(query: string, limit?: number): void { } export function validateContext(context: Context): void { - if (!context || !context.url.length) { + if (!context?.url.length) { throw new InvalidContext( "context is not defined properly - use setContext() to define the context", ); @@ -58,19 +58,19 @@ export function extractSegment( let offset; if (previewLength === -1) { // Find the last line position - const lastPos = from.trimLeft().search(LINE_BREAK); + const lastPos = from.trimStart().search(LINE_BREAK); offset = from.substring( 0, lastPos > 0 ? lastPos + 1 : from.search(LAST_LINE), ); } else { if (trimContent) { - offset = from.trimLeft().substring( + offset = from.trimStart().substring( 0, - from.trimLeft().indexOf(LINE_BREAK) + previewLength, + from.trimStart().indexOf(LINE_BREAK) + previewLength, ); } else { - offset = from.trimLeft().substring( + offset = from.trimStart().substring( 0, previewLength === 0 ? query.length : previewLength, ); @@ -83,9 +83,11 @@ export function extractSegment( /* Extract content segment from the found query to the last complete line, * However, if the previewLenght is shorter than the size of this line, it will display the established range. */ - return (trimContent - ? offset.substring(0, lastLine > 0 ? lastLine : offset.length) - : offset).trim(); + if (trimContent) { + return offset.substring(0, lastLine > 0 ? lastLine : offset.length).trim(); + } + + return offset.trim(); } /** @@ -116,5 +118,6 @@ export function findFirstPos( } return firstPos; } + return foundIndex; } diff --git a/src/skimming.ts b/src/skimming.ts index c7a8930..0e9f609 100644 --- a/src/skimming.ts +++ b/src/skimming.ts @@ -57,9 +57,8 @@ export class Skimming { } if (!results.length) { - for (let i = 0; i < this.context.files.length; i++) { - const element = this.context.files[i]; - const content = await this.readDocument(this.context.url, element); + for (const file of this.context.files) { + const content = await this.readDocument(this.context.url, file); const segment = this.skimContent( content, @@ -74,7 +73,7 @@ export class Skimming { if (segment.length) { const output = { - file: element, + file, segment, found: segment.length, cache: false, @@ -162,6 +161,8 @@ export class Skimming { }); } } + + result.body?.cancel(); throw new NotContentFound(url, doc); } } diff --git a/test/cache_test.ts b/test/cache_test.ts index 998567f..c5eb6f6 100644 --- a/test/cache_test.ts +++ b/test/cache_test.ts @@ -1,5 +1,6 @@ import { assertEquals } from "./deps.ts"; -import CacheHandle from "../src/lib/cache.ts"; +import CacheHandler from "../src/lib/cache.ts"; +import { DEFAULT_IGNORE_CASE, DEFAULT_PREVIEW_LENGTH } from "../src/skimming.ts"; const { test } = Deno; @@ -10,8 +11,11 @@ function sleep(ms: number) { test({ name: "CACHE - Should store data into cache", fn(): void { - const cacheHandle = new CacheHandle({ size: 1, expireDuration: 10 }); - cacheHandle.store( + // given + const cacheHandler = new CacheHandler({ size: 1, expireDuration: 10 }); + + // test + cacheHandler.store( "my search", { file: "filename.md", @@ -20,15 +24,18 @@ test({ cache: true, }, ); - assertEquals(cacheHandle.cache.length, 1); + assertEquals(cacheHandler.cache.length, 1); }, }); test({ name: "CACHE - Should limit cache size by 2", async fn(): Promise { - const cacheHandle = new CacheHandle({ size: 2, expireDuration: 10 }); - cacheHandle.store( + // given + const cacheHandler = new CacheHandler({ size: 2, expireDuration: 10 }); + + // test + cacheHandler.store( "my search", { file: "filename.md", @@ -37,8 +44,9 @@ test({ cache: true, }, ); + await sleep(500); - cacheHandle.store( + cacheHandler.store( "another search", { file: "filename2.md", @@ -47,10 +55,10 @@ test({ cache: true, }, ); - assertEquals(cacheHandle.cache.length, 2); + assertEquals(cacheHandler.cache.length, 2); await sleep(500); - cacheHandle.store( + cacheHandler.store( "Is there any space", { file: "filename3.md", @@ -59,15 +67,18 @@ test({ cache: true, }, ); - assertEquals(cacheHandle.cache.length, 2); + assertEquals(cacheHandler.cache.length, 2); }, }); test({ name: "CACHE - Should update exp time after accessing cached query", async fn(): Promise { - const cacheHandle = new CacheHandle({ size: 2, expireDuration: 10 }); - cacheHandle.store( + // given + const cacheHandler = new CacheHandler({ size: 2, expireDuration: 10 }); + + // test + cacheHandler.store( "my search", { file: "filename.md", @@ -76,8 +87,9 @@ test({ cache: true, }, ); + await sleep(500); - cacheHandle.store( + cacheHandler.store( "another search", { file: "filename2.md", @@ -87,19 +99,95 @@ test({ }, ); - assertEquals(cacheHandle.cache[0].exp < cacheHandle.cache[1].exp, true); + assertEquals(cacheHandler.cache[0].exp < cacheHandler.cache[1].exp, true); + + cacheHandler.fetch("my search"); + assertEquals(cacheHandler.cache[0].exp < cacheHandler.cache[1].exp, false); + }, +}); + +test({ + name: "CACHE - Should fetch from cache when ignore case enabled", + fn(): void { + // given + const cacheHandler = new CacheHandler({ size: 2, expireDuration: 10 }); + + cacheHandler.store( + "My Search", + { + file: "filename.md", + segment: ["My Search begins somewhere here"], + found: 1, + cache: true, + }, + ); + + // test + const output = cacheHandler.fetch("my search", { ignoreCase: true }); + assertEquals(output.length, 1); + assertEquals(cacheHandler.cache[0].query, "my search"); + }, +}); + +test({ + name: "CACHE - Should fetch from source once FetchOptions (previewLength) has changed", + fn(): void { + // given + const cacheHandler = new CacheHandler({ size: 2, expireDuration: 10 }); + const previewLength = 10; + + cacheHandler.store( + "My Search", + { + file: "filename.md", + segment: ["My Search begins somewhere here"], + found: 1, + cache: true, + }, + previewLength + ); + + // test + const output = cacheHandler.fetch("My Search", { previewLength: 5 }); + assertEquals(output.length, 0); + }, +}); - cacheHandle.fetch("my search"); - assertEquals(cacheHandle.cache[0].exp < cacheHandle.cache[1].exp, false); +test({ + name: "CACHE - Should fetch from source once FetchOptions (trimContent) has changed", + fn(): void { + // given + const cacheHandler = new CacheHandler({ size: 2, expireDuration: 10 }); + const trimContent = true; + + cacheHandler.store( + "My Search", + { + file: "filename.md", + segment: ["My Search begins somewhere here"], + found: 1, + cache: true, + }, + DEFAULT_PREVIEW_LENGTH, + DEFAULT_IGNORE_CASE, + trimContent + ); + + // test + const output = cacheHandler.fetch("My Search", { trimContent: false }); + assertEquals(output.length, 0); }, }); test({ name: - "CACHE - Should reorder after one element has been accessed and then another query being added", + "CACHE - Should reorder after one element has been accessed and then another query has been added", async fn(): Promise { - const cacheHandle = new CacheHandle({ size: 3, expireDuration: 10 }); - cacheHandle.store( + // given + const cacheHandler = new CacheHandler({ size: 3, expireDuration: 10 }); + + // test + cacheHandler.store( "my search", { file: "filename.md", @@ -109,7 +197,7 @@ test({ }, ); await sleep(500); - cacheHandle.store( + cacheHandler.store( "another search", { file: "filename2.md", @@ -119,17 +207,17 @@ test({ }, ); - assertEquals(cacheHandle.cache[0].query, "my search"); - assertEquals(cacheHandle.cache[1].query, "another search"); + assertEquals(cacheHandler.cache[0].query, "my search"); + assertEquals(cacheHandler.cache[1].query, "another search"); - cacheHandle.fetch("my search"); + cacheHandler.fetch("my search"); // It remains the same order because fetch does not reorder - assertEquals(cacheHandle.cache[0].query, "my search"); - assertEquals(cacheHandle.cache[1].query, "another search"); + assertEquals(cacheHandler.cache[0].query, "my search"); + assertEquals(cacheHandler.cache[1].query, "another search"); await sleep(500); - cacheHandle.store( + cacheHandler.store( "Is there any space", { file: "filename3.md", @@ -139,17 +227,20 @@ test({ }, ); - assertEquals(cacheHandle.cache[0].query, "another search"); - assertEquals(cacheHandle.cache[1].query, "my search"); - assertEquals(cacheHandle.cache[2].query, "Is there any space"); + assertEquals(cacheHandler.cache[0].query, "another search"); + assertEquals(cacheHandler.cache[1].query, "my search"); + assertEquals(cacheHandler.cache[2].query, "Is there any space"); }, }); test({ name: "CACHE - Should remove expired data from the cache", async fn(): Promise { - const cacheHandle = new CacheHandle({ size: 10, expireDuration: 1 }); - cacheHandle.store( + // given + const cacheHandler = new CacheHandler({ size: 10, expireDuration: 1 }); + + // test + cacheHandler.store( "my search", { file: "filename.md", @@ -158,8 +249,9 @@ test({ cache: true, }, ); + await sleep(1000); - cacheHandle.store( + cacheHandler.store( "another search", { file: "filename2.md", @@ -168,8 +260,9 @@ test({ cache: true, }, ); + await sleep(500); - cacheHandle.store( + cacheHandler.store( "Is there any space", { file: "filename3.md", @@ -179,8 +272,8 @@ test({ }, ); - assertEquals(cacheHandle.cache[0].query, "another search"); - assertEquals(cacheHandle.cache[1].query, "Is there any space"); - assertEquals(cacheHandle.cache.length, 2); + assertEquals(cacheHandler.cache[0].query, "another search"); + assertEquals(cacheHandler.cache[1].query, "Is there any space"); + assertEquals(cacheHandler.cache.length, 2); }, }); diff --git a/test/mod_test.ts b/test/mod_test.ts index 3a843ae..ed6567c 100644 --- a/test/mod_test.ts +++ b/test/mod_test.ts @@ -21,6 +21,7 @@ const content = ` test({ name: "MOD - Should return one valid entry", async fn(): Promise { + // given const query = "Skimming"; const files = ["README.md"]; const context: Context = { @@ -31,14 +32,43 @@ test({ const skimmer = new Skimming(); skimmer.setContext(context); + + // test const entries = await skimmer.skim(query, { previewLength: 200 }); assertEquals(entries.length, 1); }, }); +test({ + name: "MOD - Should NOT return - Exception: document not found", + async fn(): Promise { + // given + const query = "query"; + const files = ["NOT_EXIST.md"]; + const context: Context = { + url: + "https://raw.githubusercontent.com/petruki/skimming/master/test/fixtures/", + files, + }; + + const skimmer = new Skimming(); + skimmer.setContext(context); + + // test + await skimmer.skim(query).catch((error) => { + assertEquals(error.name, "NotContentFound"); + assertEquals( + error.message, + `No content found at https://raw.githubusercontent.com/petruki/skimming/master/test/fixtures/${files[0]}.`, + ); + }); + }, +}); + test({ name: "MOD - Should NOT return - Exception: empty query", fn(): void { + // given const query = ""; const files = ["README.md"]; const context: Context = { @@ -50,6 +80,7 @@ test({ const skimmer = new Skimming(); skimmer.setContext(context); + // test skimmer.skim(query).catch((error) => { assertEquals(error.name, "InvalidQuery"); assertEquals( @@ -63,8 +94,11 @@ test({ test({ name: "MOD - Should return content found not trimmed", fn(): void { + // given const query = "Lorem"; const skimmer = new Skimming(); + + // test const results = skimmer.skimContent(content, query, { previewLength: 100, trimContent: false, @@ -76,10 +110,12 @@ test({ test({ name: "MOD - Should return content found trimmed", fn(): void { + // given const query = "Lorem"; const skimmer = new Skimming(); + + // test const results = skimmer.skimContent(content, query, { previewLength: 100 }); - console.log(results); assertNotEquals(results[0].length, 100); }, }); @@ -87,8 +123,11 @@ test({ test({ name: "MOD - Should return two results - not ignore case", fn(): void { + // given const query = "Lorem"; const skimmer = new Skimming(); + + // test const results = skimmer.skimContent(content, query, { previewLength: 20 }); assertEquals(results.length, 2); }, @@ -97,8 +136,11 @@ test({ test({ name: "MOD - Should return three results - ignore case", fn(): void { + // given const query = "Lorem"; const skimmer = new Skimming(); + + // test const results = skimmer.skimContent(content, query, { previewLength: 20, ignoreCase: true, @@ -110,6 +152,7 @@ test({ test({ name: "MOD - Should NOT return - Exception: url is empty", fn(): void { + // given const files = ["README.md"]; const context: Context = { url: "", @@ -117,6 +160,8 @@ test({ }; const skimmer = new Skimming(); + + // test assertThrows( () => skimmer.setContext(context), InvalidContext, @@ -128,6 +173,7 @@ test({ test({ name: "MOD - Should NOT return - Exception: file name is empty", fn(): void { + // given const files = [""]; const context: Context = { url: @@ -136,6 +182,8 @@ test({ }; const skimmer = new Skimming(); + + // test assertThrows( () => skimmer.setContext(context), InvalidContext, @@ -147,6 +195,7 @@ test({ test({ name: "MOD - Should NOT return - Exception: endpoint might not work", fn(): void { + // given const files = ["README.md"]; const context: Context = { url: "https://raw.githubusercontent.com/petruki/skimming/master", // Here, it is missing a slash in the end @@ -154,6 +203,8 @@ test({ }; const skimmer = new Skimming(); + + // test assertThrows( () => skimmer.setContext(context), InvalidContext, @@ -165,6 +216,7 @@ test({ test({ name: "MOD - Should return value from the cache", async fn(): Promise { + // given const query = "Skimming"; const files = ["README.md"]; const context: Context = { @@ -176,6 +228,7 @@ test({ const skimmer = new Skimming({ expireDuration: 10, size: 10 }); skimmer.setContext(context); + // test let output = await skimmer.skim(query); assertEquals(output.length, 1); output.forEach((data) => { @@ -193,6 +246,7 @@ test({ test({ name: "MOD - Should return value from the cache with new preview length", async fn(): Promise { + // given const query = "Skimming({"; const files = ["README.md"]; const context: Context = { @@ -204,6 +258,7 @@ test({ const skimmer = new Skimming({ expireDuration: 10, size: 10 }); skimmer.setContext(context); + // test let output = await skimmer.skim(query, { previewLength: 20, trimContent: false, @@ -222,10 +277,47 @@ test({ }, }); +test({ + name: "MOD - Should return value from the cache with ignore case", + async fn(): Promise { + // given + const query1 = "Skimming"; + const query2 = "skimming"; + const files = ["README.md"]; + const context: Context = { + url: + "https://raw.githubusercontent.com/petruki/skimming/master/test/fixtures/", + files, + }; + + const skimmer = new Skimming({ expireDuration: 10, size: 10 }); + skimmer.setContext(context); + + // test + let output = await skimmer.skim(query1, { + previewLength: 20, + trimContent: false, + }); + + assertEquals(output.length, 1); + output.forEach((data) => assertEquals(data.segment[0].length, 20)); + + output = await skimmer.skim(query2, { + previewLength: 20, + trimContent: false, + ignoreCase: true + }); + + assertEquals(output.length, 1); + output.forEach((data) => assertEquals(data.segment[0].length, 20)); + }, +}); + test({ name: - "MOD - Should return value from OUTSIDE the cache with new preview length", + "MOD - Should return value from the source with new preview length", async fn(): Promise { + // given const query = "Skimming({"; const files = ["README.md"]; const context: Context = { @@ -237,6 +329,7 @@ test({ const skimmer = new Skimming({ expireDuration: 10, size: 10 }); skimmer.setContext(context); + // test let output = await skimmer.skim(query, { previewLength: 20, trimContent: false, @@ -265,6 +358,7 @@ test({ test({ name: "MOD - Should return value using regular expression", async fn(): Promise { + // given const query = "#{3}"; const files = ["README.md"]; const context: Context = { @@ -276,6 +370,7 @@ test({ const skimmer = new Skimming(); skimmer.setContext(context); + // test const output = await skimmer.skim(query, { previewLength: -1, regex: true, diff --git a/test/utils_test.ts b/test/utils_test.ts new file mode 100644 index 0000000..b56f235 --- /dev/null +++ b/test/utils_test.ts @@ -0,0 +1,39 @@ +import { assertEquals, assertThrows } from "./deps.ts"; +import { extractSegment, validateQuery } from "../src/lib/utils.ts"; +import { InvalidQuery } from "../src/lib/exceptions.ts"; + +const { test } = Deno; + +const content = "What is Lorem Ipsum?"; + +test({ + name: "UTILS - Should return error when limit exceeded", + fn(): void { + assertThrows(() => validateQuery("query", 3), + InvalidQuery, "Invalid query input. Cause: it exceeds the limit of 3."); + }, +}); + +test({ + name: "UTILS - Should return error when query is empty", + fn(): void { + assertThrows(() => validateQuery(""), + InvalidQuery, "Invalid query input. Cause: it is empty."); + }, +}); + +test({ + name: "UTILS - Should extract segment full line", + fn(): void { + const segment = extractSegment(content, "What", -1); + assertEquals(segment, "What is Lorem Ipsum?") + }, +}); + +test({ + name: "UTILS - Should extract segment from query lenght", + fn(): void { + const segment = extractSegment(content, "What", 0, false); + assertEquals(segment, "What") + }, +}); \ No newline at end of file