From 084897f64496e995ffba61f659fb7bc9bfc06eb3 Mon Sep 17 00:00:00 2001 From: Jonas Amundsen Date: Thu, 19 Jan 2023 16:33:35 +0100 Subject: [PATCH] Simplify handling of internal values a bit The weak map that I previously talked about [1] wasn't as I intended it to be, in this case the map only ever contain a single item. My intention was to keep a map outside of the "test's runtime", as I am doing with this change, but I now realize that strings aren't legal WeakMap keys. Furthremore, I like that the reference contained in the Cypress environment is a primitive value. Less surprises this way. Lastly, because the value known to Cypress is a primitive value and not something where we're messing with method overloading, I am more comfortable with reomving the opt-out feature of this. [1] https://github.com/badeball/cypress-cucumber-preprocessor/issues/908#issuecomment-1367864681 --- features/issues/908.feature | 34 ++++--------------- lib/assertions.ts | 8 +++-- lib/create-tests.ts | 66 ++++++++++++++----------------------- lib/methods.ts | 24 +++++++++----- 4 files changed, 51 insertions(+), 81 deletions(-) diff --git a/features/issues/908.feature b/features/issues/908.feature index 2339eb3b..e8eed41d 100644 --- a/features/issues/908.feature +++ b/features/issues/908.feature @@ -6,40 +6,18 @@ Feature: hide internals from cypress environment """ Feature: a feature Scenario: hide internal state by default - * the internal state should not be visible - - @reportInternalCucumberState - Scenario: expose internal state with tag - * the internal state should be visible + Then the visible internal state should be a mere reference """ And a file named "cypress/support/step_definitions/steps.js" with: """ - const { inspect } = require('util'); const { Then } = require("@badeball/cypress-cucumber-preprocessor"); const { INTERNAL_SPEC_PROPERTIES } = require("@badeball/cypress-cucumber-preprocessor/lib/constants"); - - Then("the internal state should not be visible", () => { + // From https://github.com/rfrench/chai-uuid/blob/master/index.js. + const UUID_V4_EXPR = /^[0-9A-F]{8}-[0-9A-F]{4}-[4][0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$/i; + Then("the visible internal state should be a mere reference", () => { const properties = Cypress.env(INTERNAL_SPEC_PROPERTIES); - const serializedProperties = JSON.stringify(properties); - const inspectedProperties = inspect(properties, {depth: 0}); - - expect(properties).not.to.be.undefined; - expect(properties.pickle).to.be.undefined; - expect(serializedProperties).to.be.undefined; - expect(inspectedProperties).to.be.equal('{}'); - }); - - Then("the internal state should be visible", () => { - const properties = Cypress.env(INTERNAL_SPEC_PROPERTIES); - const serializedProperties = JSON.stringify(properties); - const inspectedProperties = inspect(properties, {depth: 0}); - - expect(properties).not.to.be.undefined; - expect(properties.pickle).not.to.be.undefined; - expect(serializedProperties).not.to.be.undefined; - // Must be more than an empty object, cannot test the exact string because - // it contains random values in a random order. - expect(inspectedProperties).to.have.length.of.at.least(3); + expect(properties).to.be.a("string"); + expect(properties).to.match(UUID_V4_EXPR); }); """ When I run cypress diff --git a/lib/assertions.ts b/lib/assertions.ts index aa7ba33e..42ba3467 100644 --- a/lib/assertions.ts +++ b/lib/assertions.ts @@ -2,12 +2,16 @@ import { isString } from "./type-guards"; import { homepage } from "../package.json"; -export function fail(message: string) { - throw new Error( +export function createError(message: string) { + return new Error( `${message} (this might be a bug, please report at ${homepage})` ); } +export function fail(message: string) { + throw createError(message); +} + export function assert(value: unknown, message: string): asserts value { if (value != null) { return; diff --git a/lib/create-tests.ts b/lib/create-tests.ts index f123be08..26d1b52a 100644 --- a/lib/create-tests.ts +++ b/lib/create-tests.ts @@ -114,48 +114,29 @@ export interface InternalSuiteProperties { isEventHandlersAttached?: boolean; } -class InternalSpecPropertiesReference { - private static referenceMap = new WeakMap< - InternalSpecPropertiesReference, - InternalSpecProperties - >(); - - private constructor() { - // Empty constructor just to set private visibility - } - - public static new( - scenarioName: string, - properties: InternalSpecProperties - ): InternalSpecPropertiesReference { - const reference = new this(); - - this.referenceMap.set(reference, properties); - - return reference; - } - - public dereference(): InternalSpecProperties { - // Non-null assertion is safe because an instance of reference is only given out after it is included - // in the reference map. - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - return InternalSpecPropertiesReference.referenceMap.get(this)!; - } - - public toJSON(): void { - // Noop override to hide the reference from serialization. - // Any property pointing to a reference will be ignored by JSON.stringify - } +const internalSpecProperties = new Map(); + +function createInternalSpecProperties( + properties: InternalSpecProperties +): string { + const reference = uuid(); + internalSpecProperties.set(reference, properties); + return reference; } export function retrieveInternalSpecProperties(): InternalSpecProperties { - const envValue = Cypress.env(INTERNAL_SPEC_PROPERTIES); + const reference = Cypress.env(INTERNAL_SPEC_PROPERTIES) as string; - if (envValue instanceof InternalSpecPropertiesReference) { - return envValue.dereference(); - } + return assertAndReturn( + internalSpecProperties.get(reference), + `Expected to find internal spec properties with reference = ${reference}` + ); +} - return envValue; +function updateInternalSpecProperties( + newProperties: Partial +): void { + Object.assign(retrieveInternalSpecProperties(), newProperties); } function retrieveInternalSuiteProperties(): @@ -361,9 +342,8 @@ function createPickle( }; const internalEnv = { - [INTERNAL_SPEC_PROPERTIES]: tags.includes("@reportInternalCucumberState") - ? internalProperties - : InternalSpecPropertiesReference.new(scenarioName, internalProperties), + [INTERNAL_SPEC_PROPERTIES]: + createInternalSpecProperties(internalProperties), }; const suiteOptions = tags @@ -819,8 +799,10 @@ export default function createTests( /** * Repopulate internal properties in case previous test is retried. */ - properties.testCaseStartedId = uuid(); - properties.remainingSteps = [...properties.allSteps]; + updateInternalSpecProperties({ + testCaseStartedId: uuid(), + remainingSteps: [...properties.allSteps], + }); }); after(function () { diff --git a/lib/methods.ts b/lib/methods.ts index 0caa1e98..ea94561c 100644 --- a/lib/methods.ts +++ b/lib/methods.ts @@ -1,16 +1,19 @@ +import { Pickle } from "@cucumber/messages"; + import parse from "@cucumber/tag-expressions"; + import { fromByteArray } from "base64-js"; -import { assertAndReturn } from "./assertions"; + +import { createError } from "./assertions"; + import { collectTagNames } from "./ast-helpers"; import { INTERNAL_SPEC_PROPERTIES, TASK_CREATE_STRING_ATTACHMENT, } from "./constants"; -import { - InternalSpecProperties, - retrieveInternalSpecProperties, -} from "./create-tests"; + +import { retrieveInternalSpecProperties } from "./create-tests"; import { runStepWithLogGroup } from "./cypress"; @@ -123,10 +126,13 @@ export const NOT_FEATURE_ERROR = "Expected to find internal properties, but didn't. This is likely because you're calling doesFeatureMatch() in a non-feature spec. Use doesFeatureMatch() in combination with isFeature() if you have both feature and non-feature specs"; function doesFeatureMatch(expression: string) { - const { pickle } = assertAndReturn( - retrieveInternalSpecProperties(), - NOT_FEATURE_ERROR - ) as InternalSpecProperties; + let pickle: Pickle; + + try { + pickle = retrieveInternalSpecProperties().pickle; + } catch { + throw createError(NOT_FEATURE_ERROR); + } return parse(expression).evaluate(collectTagNames(pickle.tags)); }