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

[RFC] Initial design #1

Open
lolmaus opened this issue May 16, 2020 · 9 comments
Open

[RFC] Initial design #1

lolmaus opened this issue May 16, 2020 · 9 comments
Assignees

Comments

@lolmaus
Copy link
Contributor

lolmaus commented May 16, 2020

File locations

Path Use Default import must return
/tests/yadda/steps.js Step library to be used for feature files A hash of opinionated steps
/tests/yadda/steps/<my-steps>.js Custom step implementations A hash of opinionated steps, to be composed into main steps.js
/tests/yadda/dictionary.js Converters aka macros An hash of string | Regexp | [string | RegExp, (string[]) => Promise<any> ]
/tests/yadda/labels.js Custom labels for third-party selectors An hash of string, e. g. {'Bootstrap-Primary-Button', '.btn-primary')
/tests/yadda/annotations.js Annotations to setup features and scenarios TBD

Populating the dictionary

Yadda has a weird API for defining the dictionary:

new Yadda.Dictionary()
  .define('address', '$street, $postcode')
  .define('street', /(\d+) (\w+))
  .define('postcode', /((GIR &0AA)|((([A-PR-UWYZ][A-HK-Y]?[0-9][0-9]?)|(([A-PR-UWYZ][0-9][A-HJKSTUW])|([A-PR-UWYZ][A-HK-Y][0-9][ABEHMNPRV-Y]))) &[0-9][ABD-HJLNP-UW-Z]{2}))/)
  .define('num', /(\d+)/, Yadda.converters.integer)
  .define('quantity', /(\d+) (\w+)/, (amount, units, cb) => {
    cb(null, { amount: amount, units: units })
  });
  .define('user', '$user', (userName, cb) => {
    fetch(`https://api.example.com/users/${userName}`).then(
      (response) => cb(null, response.json()),
      (e) => cb(e)
    );
  });

This Node-style callback is inconvenient and hard to type. Basically, you pass a callback that accepts a callback.

ember-yadda uses a simplified way of defining the dictionary:

{
  address: '$street, $postcode',
  street: /(\d+) (\w+),
  postcode: /((GIR &0AA)|((([A-PR-UWYZ][A-HK-Y]?[0-9][0-9]?)|(([A-PR-UWYZ][0-9][A-HJKSTUW])|([A-PR-UWYZ][A-HK-Y][0-9][ABEHMNPRV-Y]))) &[0-9][ABD-HJLNP-UW-Z]{2}))/,
  num: Yadda.converters.integer,

  quantity: [
    /(\d+) (\w+)/,
    (amount, units) => ({ amount: amount, units: units })
  ],

  user: [
    '$user',
    async (userName) => (await fetch(`https://api.example.com/users/${userName}`)).json()
  ]
}

Under the hood, ember-yadda will convert these straightforward callbacks into Node-style supported by Yadda like this:

import opinionatedDictionary from '<my-app>/tests/yadda/dictionary';

function instantiateYaddaDictionary(opinionatedDictionary) {
  const yaddaDictionary = new Yadda.Dictionary();

  Object.entries(opinionatedDictionary).forEach(([macroName, macroDefinition]) => {
    if (Array.isArray(macroDefinition)) {
      const [pattern, converter] = macroDefinition;
      yaddaDictionary.define(macroName, pattern, wrapConverterWithNodeStyle(converter));
    } else {
      yaddaDictionary.define(macroName, macroDefinition);
    }
  });

  return yaddaDictionary;
}

function wrapConverterWithNodeStyle(converter) {
  return (...args) => {
    const cb = args.pop();

    try {
      const result = await converter(...args);
      cb(null, result);
    } catch (e) {
      cb(e);
    }
  }
}

export default instantiateYaddaDictionary(opinionatedDictionary);

Now it is very convenient to define converter macros, but existing converters from Yadda.converters are Node-style. I see three ways to resolve this:

  1. When defining a macro using a Yadda.converters converter, use an util on it so that it is wrapped with async/await that the addon supports.

    E. g. instead of:

    num: Yadda.converters.integer,

    you would do:

    num: converter(Yadda.converters.integer),

    The downside of this approach is that a Node-style converter will be wrapped with async/await by the util and then wrapped again with Node-style by the addon.

  2. Have wrapConverterWithNodeStyle detect whether the converter is Node-style or async/await.

    One way to do this would be to use introspection:

    import introspect from 'introspect-fun'; // https://github.com/NicolasVargas/introspect-fun
    
    function maybeWrapConverterWithNodeStyle(converter) {
      return introspect(coverter).pop() === 'next'
        ? converter
        : wrapConverterWithNodeStyle(converter);
    }
    

    The downside of this approach is that it's magical and unreliable, since it assumes that a node-style callback has its last argument called next. It will fail both when a Yadda.converters converter has the last argument called differently (currently not the case) and when an async/await converter has its last argument called next.

    There's also a performance implication, but it should be negligible.

    Maybe there's another way to distinguish Node-style Yadda.converters converters from async/await converters?

  3. Simply port all six Yadda.converters converters, so that either the ember-yadda addon or a separate content-oriented addon offers async/await equivalents.

    Downside: more work to do. I was gonna write "more maintenance", but after ensuring test coverage the code will not need to be revisited.

    We'll also need to maintain compatibility with Yadda.converters, but I doubt that they will stray far from what they currently are.

Supporting legacy ember-cli-yadda

In the legacy ember-cli-yadda, a test file generated from a feature file imports a matching steps file like this.

We could make this import conditional. If the steps file exists, run it using the legacy setup. Otherwise, use the new setup described above.

Within the steps file, the user has complete freedom. They can:

  1. Keep using the new setup, but override some of the opinionated steps. This could be useful to resolve step name conflicts between opinionated step libraries.
  2. Use the legacy approach just like in ember-cli-yadda.
  3. Use a mixed approach via tricks from ember-cli-yadda-opinionated. This lets you use legacy ember-cli-yadda steps and opinionated steps in the same feature.

Things not covered

Localization

Previously, localizaton setup was happening in the app.

Due to drastically simplified design of addon's in-app files, the addon will have yadda.localisation.default hardcoded.

I believe internationalized steps are a bad practice, so we should not bother making it configurable.

In future, if there's a feature request for supporting non-English locales, we'll be able to think about a way of making it configurable.

@simonihmig
Copy link
Member

/tests/yadda/labels.js seems to be related exclusively to the ember-cli-yadda-opinionated addon, so not sure if ember-yadda should be concerned about that?

Regarding callback-style vs. async/await, detecting which style is used should be easy. Mocha does the same, see https://mochajs.org/#asynchronous-code. I think the way would be to check the (sync) return value of the function. If it is a promise (i.e. has a .then member function), then treat it as async, otherwise assume it expects a callback.

It's not clear to me which step implementation gets imported for a given feature. Is /tests/yadda/steps/<my-steps>.js supposed to match a my-steps.feature file? Or how is <my-steps>.js supposed to be used? And will /tests/yadda/steps.js be always available, automatically for every feature?

Where should feature files live, also under /tests/yadda instead of /tests/acceptance?

Agree we should not spend time on localization, never ever used it...

@lolmaus
Copy link
Contributor Author

lolmaus commented May 16, 2020

/tests/yadda/labels.js seems to be related exclusively to the ember-cli-yadda-opinionated addon, so not sure if ember-yadda should be concerned about that?

My plan was for ember-yadda to be opinionated.

Basically, I would like to retire ember-cli-yadda-opinionated and split it into:

  • ember-yadda — a testing framework with all the tools
  • A bunch of minimal addons with step libraries:
    • ember-yadda-steps — standard opinionated steps
    • ember-yadda-steps-mirage
    • ember-yadda-steps-window-mock
    • ember-yadda-steps-power-select
    • ember-yadda-steps-power-date-picker
    • ember-yadda-steps-clipboard
    • ember-yadda-steps-configuration-service
    • etc

This splitting is similar to:


If it is a promise (i.e. has a .then member function), then treat it as async, otherwise assume it expects a callback.

That's awesome, thanks!


It's not clear to me which step implementation gets imported for a given feature. Is /tests/yadda/steps/<my-steps>.js supposed to match a my-steps.feature file? Or how is <my-steps>.js supposed to be used? And will /tests/yadda/steps.js be always available, automatically for every feature?

A <name>-test.js file, which is generated from a <name>.feature file, will check whether a tests/acceptance/steps/<name>-steps.js file exists.

If it does, then it will be used (allowing for legacy ember-cli-yadda approach, mixed approach or customized opinionated approach).

If a steps file does not exist, then a default set of steps will be loaded from tests/yadda/steps.js. This set of steps will be pre-instantiated, so that a single instance will be shared across all opinionated features. We already do that in WBC and TLW.


Where should feature files live, also under /tests/yadda instead of /tests/acceptance?

Under /tests/acceptance/. The only difference with ember-cli-yadda in this regard will be that a foo.feature will not conflict with foo-test.js.

@simonihmig
Copy link
Member

My plan was for ember-yadda to be opinionated.

Not sure what exactly this means in this context...

Basically, I would like to retire ember-cli-yadda-opinionated and split it into:

Ok, that's one thing, but does that mean that ember-yadda would provide the selector features that ember-cli-yadda-opinionated currently has? Or wouldn't that be ember-yadda-steps?

So basically still unsure why the testing framework (ember-yadda) should be concerned with the concept of "labels"?

That aside, the idea of separate minimal addons seams reasonable. Two things come to my mind:

  • we could publish them under a common namespace, like @ember-yadda/core, @ember-yadda/steps, @ember-yadda/steps-mirage.
  • all those packages could still be organized in a single mono-repo. I don't have first hand experience with that approach yet, hearing mixed opinions. But it seems like a proper use case for that approach here, as it prevents you from having to yarn link addons together all the time, as I suppose they will depend on each other heavily, and sometimes changes need to be done in more than one package. yarn workspaces and lerna are the tools to use there AFAIK.

See e.g orbit.js for a project doing both of these approaches!

A -test.js file, which is generated from a .feature file, will check whether a tests/acceptance/steps/-steps.js file exists.
If it does, then it will be used

Does that mean it would have to import and mix-in the generic steps from /tests/yadda/steps.js? Or does that happen implicitly (which would be different from current ember-cli-yadda)

Under /tests/acceptance/. The only difference with ember-cli-yadda in this regard will be that a foo.feature will not conflict with foo-test.js.

How would that work (the non-conflicting part)?

Also worried if feature and step files shouldn't be closer together (aka co-located)? But maybe it's ok, more thinking aloud...

@lolmaus
Copy link
Contributor Author

lolmaus commented May 18, 2020

Ok, that's one thing, but does that mean that ember-yadda would provide the selector features that ember-cli-yadda-opinionated currently has? Or wouldn't that be ember-yadda-steps?

Both approaches could work.

On the one hand, putting the labels logic into ember-yadda will let ember-yadda-steps-* addons depend only on ember-yadda and not on ember-yadda-steps.

One the other hand, putting the labels logic into ember-yadda-steps will result in cleaner separation of concerns.

But I believe that it's easier to teach and causing less frustration when everything is in one place.

If labels logic is in ember-yadda-steps, should the steps logic also be in ember-yadda-steps?

If yes, then the ember-yadda addon should be able to operate without ember-yadda-steps. I'm not sure what it's gonna do without it. Not even sure how to implement it, need a plugin system or something, because this part will need to behave differently depending on whether ember-yadda-steps is present.

If no, the why put part of ember-yadda-steps logic into one addon and another part in another addon?

Let's make ember-yadda to contain all the logic and steps addon only contain steps implementations without any logic.

@lolmaus
Copy link
Contributor Author

lolmaus commented May 18, 2020

  • we could publish them under a common namespace, like @ember-yadda/core, @ember-yadda/steps, @ember-yadda/steps-mirage.

👍


  • all those packages could still be organized in a single mono-repo. I don't have first hand experience with that approach yet, hearing mixed opinions. But it seems like a proper use case for that approach here, as it prevents you from having to yarn link addons together all the time, as I suppose they will depend on each other heavily, and sometimes changes need to be done in more than one package. yarn workspaces and lerna are the tools to use there AFAIK.

I also have no experience with monorepos and I've seen projects having FAQ entries explaining why they don't want to be monorepos, so initially I was hesitant about it.

I think monorepos are appropriate for projects where a big app is based on multiple self-owned dependencies. We have the opposite: a number of child addons, each depending on a core addon.

But after reading a few posts on monorepos and giving it a thought, I'm ready to try it out.

The codebase is gonna be small enough for many of monorepo downsides to be irrelevant, and making breaking changes should require much less effort. Also, new experience!

@lolmaus
Copy link
Contributor Author

lolmaus commented May 18, 2020

A <name>-test.js file, which is generated from a <name>.feature file, will check whether a tests/acceptance/steps/<name>-steps.js file exists.

Does that mean it would have to import and mix-in the generic steps from /tests/yadda/steps.js? Or does that happen implicitly (which would be different from current ember-cli-yadda)

If tests/acceptance/steps/<name>-steps.js does not exist, then /tests/yadda/steps.js will be imported. /tests/yadda/steps.js exports a pre-instantiated library of steps (but also has a named export of the opinionated hash of steps).

If tests/acceptance/steps/<name>-steps.js exists, then it will be imported and used in the test module.

By adjusting the content of tests/acceptance/steps/<name>-steps.js , the user has precise control over what will be done. The user has a number of options:

  1. reexport /tests/yadda/steps.js (same as not having the file);
  2. import a named hash of opinionated steps from /tests/yadda/steps.js, manipulate it, then instantiate it with an util from ember-yadda;
  3. import ember-cli-yadda legacy steps factory from /tests/acceptance/steps/steps.js and instantiate it;
  4. import both opinionated steps hash and legacy steps factory and compose them using an util from ember-yadda.

@lolmaus
Copy link
Contributor Author

lolmaus commented May 18, 2020

Under /tests/acceptance/. The only difference with ember-cli-yadda in this regard will be that a foo.feature will not conflict with foo-test.js.

How would that work (the non-conflicting part)?

Instead of turning a /tests/acceptance/foo.feature into /tests/acceptance/foo-test.js, it will be turned into /tests/acceptance/__ember-yadda__-foo-test.js or something like that.


Also worried if feature and step files shouldn't be closer together (aka co-located)? But maybe it's ok, more thinking aloud...

The opinionated approach does not suppose having per-feature steps files.

That's unless you're gonna do number 2 from the previous comment, but that's more like an escape hatch for a non-standard situation rather than a normal workflow. And if you do that, no one is stopping you from placing your per-feature steps next to the feature. It's entirely up to the user.

The legacy ember-cli-yadda approach already has step files non-collocated. We only want to support it for backwards comaptibility, no reason to make a modernized legacy approach...

@lolmaus
Copy link
Contributor Author

lolmaus commented May 18, 2020

@simonihmig This article says that monorepos are awesome, but Lerna approach to monorepos is bad: https://blog.nrwl.io/misconceptions-about-monorepos-monorepo-monolith-df1250d4b03c

@lolmaus
Copy link
Contributor Author

lolmaus commented May 18, 2020

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

No branches or pull requests

4 participants