Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Strict Step Registration #240

Open
Aaron-Pool opened this issue Oct 31, 2024 · 4 comments
Open

Feature: Strict Step Registration #240

Aaron-Pool opened this issue Oct 31, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@Aaron-Pool
Copy link

The problem
When it comes to code dependencies, I love traceability, and the fact that a step registered by a Given returned from a different invocation createBdd(test) can be used in any generated test can often be a blocker to setting up traceable step sharing. Some people may prefer this ease of sharing, as it mimics the step sharing in cucumber, but personally, I prefer to group my steps by domain and require any test suite that wants to use those steps to explicitly call a registration type function.

For example, a step.ts file I write for a high level feature might look like this:

// entityEditor defines the base "test" instance that a "test" instance must extend
// to use these steps
export const registerInventoryManager = shareSteps(entityEditor, ({ Given }) => {
  Given(
    /I am on the "(.*)" page of "(.*)"/,
    async ({ page, tab, getIdByName, id }, route: string, name: string) => {
      const _id = getIdByName(name, entries);
      id.set(_id);
      await page.goto(`/admin/inventory/${id.get()}${route}`);
      return tab('current').waitFor();
    }
  );
  Given(/I am on the "(.*)" page of a new inventory entry/, async ({ page, tab, id }, route: string) => {
    id.set(unsavedIdentifier);
    await page.goto(`/admin/inventory/new${route}`);
    return tab('current').waitFor();
  });
});

const { Given, When, Then } = registerInventoryManager(createBdd(test));

Given('the entry {string} exists', async ({}, Name) => {
  const newEntry = entry.factory({ Name });
  entries.tap((store) => store.set(newEntry.Id, [newEntry]));
});

Given('{string} has the item {string}', async ({}, entryName: string, itemName: string) => {
  entries.tap((store) => {
    const [entryId, list] = [...store.entries()].find(([, list]) =>
      list.find((q) => q.Name === entryName)
    ) ?? [null, null];
    if (!entryId || !list) throw new Error('target entry not found!');
    const updated = list.map((q) => {
      if (q.Name !== entryName) return q;
      const newItem = item({ name: itemName });
      return { ...q, items: q.items.concat(newItem) };
    });
    store.set(entryId, updated);
  });
});

Certain of these steps are usable by other, more granular tests that are in different suites. I do want to expose those steps to be sharable (though only by explicit registration). Other steps in this file are more specific and I want them to not be usable elsewhere. This makes it very straight forward to find the specific step definition a given tests is using by tracing the import path in an IDE. It also prevents step discovery that is semi-"magical".

The above shareSteps utility is a small helper I wrote to create a registration function, and it looks like:

import { createBdd, TestType } from '@resolution/playwright';

type BDD<T extends object, W extends object> = ReturnType<typeof createBdd<T, W>>;
const registered = new Map<unknown, Set<unknown>>();

export function shareSteps<BT extends object, BW extends object>(
  _: TestType<BT, BW>,
  registrationFn: (bdd: BDD<BT, BW>) => void
) {
  return <DerivedBDD extends BDD<BT, BW>>(bdd: DerivedBDD) => {
    if (registered.has(bdd) && registered.get(bdd)?.has(registrationFn)) return bdd;
    if (!registered.has(bdd)) registered.set(bdd, new Set());
    registrationFn(bdd);
    registered.get(bdd)?.add(registrationFn);
    return bdd;
  };
}

This is fine, but it only creates the illusion of traceability, because the other, non-shared steps are still visible and available to any steps file that imports the first file, even if they only imported the file to use the register* function.

A solution

The solution I had in mind would be to add a strictRegistration option to the input configuration, which scopes the StepRegistry for each created instance returned by createBdd?

@Aaron-Pool Aaron-Pool added the enhancement New feature or request label Oct 31, 2024
@vitalets
Copy link
Owner

vitalets commented Nov 1, 2024

Would not step tagging per #205 be a solution for that?

To have steps exclusively bound to entityEditor, you can define them with tag:

Given(/I am on the "(.*)" page of "(.*)"/, { tags: '@entityEditor' }, async () => { ... });
Given(/I am on the "(.*)" page of a new inventory entry/, { tags: '@entityEditor' }, async () => { ... });

And in the feature file:

@entityEditor
Feature: Entity Editor

  ...

Other steps without tag are shareable across all features.

I personally like even more the idea in #217, so we can put all entityEditor stuff into tagged folder and steps will be automatically bound to it:

features
└── @entityEditor
    ├── entityEditor.feature
    └── steps.ts

All steps from @entityEditor/steps.ts can be used only in entityEditor. If other suites need them, they should explicitly have @entityEditor tag.

Some other questions, to better understand the context:

  1. Certain of these steps are usable by other... Other steps in this file are more specific

    Would not it better to split shareable and non-shareable steps into two different files?

  2. I do want to expose those steps to be sharable (though only by explicit registration)

    Can tags serve as explicit registration of using steps? Otherwise, as a feature-writer how can I know which steps can I use for the particular feature?

  3. This makes it very straight forward to find the specific step definition a given tests is using by tracing the import path in an IDE.

    Do you start tracing from feature file by clicking on the step?

@Aaron-Pool
Copy link
Author

Aaron-Pool commented Nov 1, 2024

@vitalets Thank you for your speedy response and detailed thoughts. I'll try address these thoughts individually. Ultimately, I completely respect the decisions you've made and your overall approach with playwright-bdd, so I'm just going to try to outline the outcome I see as desirable, and if it seems reasonable to you, then that's great, and if not, I get it.

Would not it better to split shareable and non-shareable steps into two different files?

I did consider this, but ultimately I feel that traceability is such a "solved problem" on the javascript/typescript side, so to me it seems the most logical to use code, rather than file structure or tagging conventions, to drive traceability. Because of this, I organize my tests in pairs of *.feature and step.ts files. And the mental model I try to communicate to the junior devs that I manage is that the step.ts file that lives beside a *.feature file is the source of truth for what Given/Then/When statements are available in that feature file. If the Given in a step.ts file is not provided a definition of a specific step by some means, it won't be able to run that step in the feature file. Which, wise or unwise, is my attempt to make the "magic gap" between the .feature and .ts worlds as small as possible (e.g. - just between two co-located files). So even though in this specific example splitting the files would prevent the incidental step registration, it doesn't resolve the overarching issue of incidental step registration happening due to import side effects 🤷‍♂️

Can tags serve as explicit registration of using steps? Otherwise, as a feature-writer how can I know which steps can I use for the particular feature?

My team specifically doesn't have any dedicated "feature writers". The same devs who automate tests write feature files, and our QA specialists/Analysts simply review those feature files for gaps/unconsidered use cases.

Do you start tracing from feature file by clicking on the step?

Personally, I do not, no. Because most IDE tools that enable this feature, to my knowledge, rely on globs and implicit attachment of step definitions to feature files. And perhaps I'm overly overparticular about this, but I really value explicit dependencies over implicit dependencies. It's probably a result of trauma from my experiences in the early 2010s pre-modular javascript world.

@vitalets
Copy link
Owner

@Aaron-Pool

Thought more about that request. So the goal is to have steps.ts next to the .feature file and restrict usage of any other steps?

To me #217 still looks like a solution. Imagine the following structure:

features
└── @entityEditor
    ├── entityEditor.feature
    └── steps.ts
└── @anotherFeature
    ├── anotherFeature.feature
    └── steps.ts

All steps will be bound to the respective feature. Or I'm missing something?

@Aaron-Pool
Copy link
Author

Aaron-Pool commented Nov 27, 2024

Thought more about that request. So the goal is to have steps.ts next to the .feature file and restrict usage of any other steps?

This, but also the ability to explicitly register/share some or all steps of a different "feature". So, with the above structure it would be something like

// @entityEditor/steps.ts
import { createBdd } from 'playwright-bdd';

export const entityEditor = createBdd();

entityEditor.Given(...)

entityEditor.Then(...)

const { Given, When, Then } = createBdd().use(entityEditor);

Given(....)

When(...)

Then(....)
// @anotherFeature/steps.ts
import { entityEditor } from '../@entityEditor';

const { Given, When, Then } = createBdd(...).use(entityEditor);

Given(....)

When(...)

Then(....)

The important thing being that I couldn't accidentally register steps just by the fact that /@entityEditor had somehow been incidentally imported into the bundle.

I have made my original solution semi-workable by just making sure that every shareSteps call is in its own file, like you suggested. It just means that sharable steps and "private" steps can't be co-located in the same file 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants