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

document fillInEditor test helper #480

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

jelhan
Copy link

@jelhan jelhan commented Jun 14, 2022

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. 😇

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
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
Copy link
Collaborator

miguelcobain commented Jun 15, 2022

@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
Collaborator

@jelhan friendly reminder of this issue. 😄

@jelhan
Copy link
Author

jelhan commented Dec 17, 2022

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
Copy link
Collaborator

miguelcobain commented Dec 22, 2022

@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
Copy link
Author

jelhan commented Dec 22, 2022

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
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