Skip to content

document fillInEditor test helper#480

Open
jelhan wants to merge 1 commit into
evocount:masterfrom
jelhan:patch-1
Open

document fillInEditor test helper#480
jelhan wants to merge 1 commit into
evocount:masterfrom
jelhan:patch-1

Conversation

@jelhan

@jelhan jelhan commented Jun 14, 2022

Copy link
Copy Markdown

Documents the new fillInEditor test helper as discussed in #477.

Please let me know if you see need for additional explanation. The test helper seems to be straight forward to me. But maybe I missed important aspects, which should be documented beside its existence. 😇

Comment thread README.md
A test helper `fillInEditor` is exported from `ember-tui-editor/test-support/helpers`, which could be used in integration and acceptance tests:

```js
import { fillInEditor } from 'ember-tui-editor/test-support/helpers';

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if the export path should be maybe simplified to ember-tui-editor/test-support. I guess it would ease discoverability of that test helper.

@miguelcobain

miguelcobain commented Jun 15, 2022

Copy link
Copy Markdown
Collaborator

@jelhan perhaps you could improve the example with a real test? I think that is helpful because users will immediately feel "at home" when they see a familiar testing environment.

Regarding the import path being ember-tui-editor/test-support/helpers, I see it as follows:

  • ember-tui-editor - the addon
  • test-support - test related things
  • helpers - the helpers

In theory, we could export other things that aren't helpers (maybe setup functions). I also see this pattern used in other addons.
I'm open to suggestions, though.

@miguelcobain

Copy link
Copy Markdown
Collaborator

@jelhan friendly reminder of this issue. 😄

@jelhan

jelhan commented Dec 17, 2022

Copy link
Copy Markdown
Author

I'm sorry that I haven't come back to this earlier. I had it on my todo list for a long-time. But it always stayed at the bottom.

@jelhan perhaps you could improve the example with a real test? I think that is helpful because users will immediately feel "at home" when they see a familiar testing environment.

I'm not entirely how a meaningful example would look like.

We expect an user to use the fillInEditor helper to test a feature, which is implemented using <TuiEditor>. To have a meaningful example test, we would first need to introduce an example using <TuiEditor> within another component. Additionally such a test would require all boilerplate for tests in Ember. I fear the ratio between the code being specific about fillInEditor and the other code would be really bad. It may make it more difficult for an user to find the relevant lines to understand fillInEditor.

Maybe we could add a link to tests/integration/components/tui-editor-test.js instead for demonstrating real-world usage?

Regarding the import path being ember-tui-editor/test-support/helpers, I see it as follows:

* `ember-tui-editor` - the addon
* `test-support` - test related things
* `helpers` - the helpers

In theory, we could export other things that aren't helpers (maybe setup functions). I also see this patterned used in other addons. I'm open to suggestions, though.

Personally I don't see an issue by exporting a setup method and test helpers from the same module. An example for this addon assuming it requires a setup method at some time would look like this:

import { setupTuiEditor, fillInEditor } from 'ember-tui-editor/test-support';

I think that is easier to read and discover than:

import { setupTuiEditor } from 'ember-tui-editor/test-support/setup';
import { fillInEditor } from 'ember-tui-editor/test-support/helpers';

Please note that Ember CLI Mirage is also exporting the setupMirage method from ember-cli-mirage/test-support since some time: https://github.com/miragejs/ember-cli-mirage/releases/tag/v1.0.0-beta.2 The example you shared seems to use an outdated version of Ember CLI Mirage.

@miguelcobain

miguelcobain commented Dec 22, 2022

Copy link
Copy Markdown
Collaborator

@jelhan regarding the example, I meant all the usual boilerplate test code, but I see your point. It becomes harder to see what exactly we are providing with all that code.

I'm convinced by your arguments in favor or import { fillInEditor } from 'ember-tui-editor/test-support';. Would you be willing to add that to the PR? Perhaps we could keep the current file for backwards compatibility.

@jelhan

jelhan commented Dec 22, 2022

Copy link
Copy Markdown
Author

I'm convinced by your arguments in favor or import { fillInEditor } from 'ember-tui-editor/test-support';. Would you be willing to add that to the PR? Perhaps we could keep the current file for backwards compatibility.

Sure. But I would recommend doing it in a separate PR to keep each of them focused.

Should the old export be deprecated? Or would you prefer to keep both long-term?

@miguelcobain

Copy link
Copy Markdown
Collaborator

@jelhan I think deprecating would be ideal.

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

Successfully merging this pull request may close these issues.

2 participants