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

Add some helpful utilities #42

Merged
merged 6 commits into from
Feb 2, 2022

Conversation

NullVoxPopuli
Copy link
Contributor

#41

@bendemboski
Copy link
Owner

Oh, sorry @NullVoxPopuli I think we misunderstood each other in the discussion in #41. I'm happy to continue discussing this, but I was saying that especially with RFC 726 pending, I don't really want to build utilities into fractal-page-object that make assertions about or interact with the DOM and are therefore likely to be redundant with libraries like qunit-dom or @ember/test-helpers, but rather use RFC 726 to integrate with such libraries and expose whatever primitives are needed for users/other libraries to build such functionality on top of fractal-page-object.

So I was saying, I'd definitely be into exporting a getDescription(obj: PageObject): string function from fractal-page-object, but I'd prefer to either leave helpers such as text and assertExists to user to implement, or put them in a separate library (although I think I'd be fine with putting it in this monorepo, so it's part of the larger project, just kept separate from the fractal-page-object core).

@NullVoxPopuli
Copy link
Contributor Author

qunit-dom and test-helpers don't do anything with actually authoring page-objects though. Like, you need utilities like this regardless of RFC 726 landing

@bendemboski
Copy link
Owner

I see. Okay, I'm sold on assertExists. Not really on text though -- do you have an argument for including that one in the fractal core?

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Jan 27, 2022

do you have an argument for including that one in the fractal core?

couple:

  • it's still query, non-action
  • sometimes you want page-objects' properties to return text directly, rather than the element, because some DOM is private API

In my case, for some libraries, I'm considering all DOM is private API, and only using .element for my own tests internally.
The reasoning is:

  • I can provide a more reasonable API than what we have today (super high level) -- this could change with the aforementioned RFC, like making an element public API, but not the path to it (as is common with page-object-abuse (DOM-tree as public object-api lololol 😢 )
    • this allows for me to totally change implementation details, elements, etc without making my consumers worry

but text is tiny enough where I could be ok with excluding it from this PR -- I'd probably want to PR to @ember/test-helpers to add the utliity. The main thing, is that the native way to query the text on an element is verbose

@bendemboski
Copy link
Owner

Got it. I'm resistant to text but comfortable with assertExists being in the core because:

  1. text is solving for an awkwardness in the DOM APIs that has nothing at all to do with fractal-page-object, while assertExists is solving for an awkwardness in how page objects are put together since their element may or may not exist
  2. text seems like a slippery slope -- what about the values of text inputs or check-ed-ness of checkboxes or the hidden-ness of inputs etc.? I know text is probably used a lot more often, but still slippery slope...

It's harder for me to think about text as part of the necessary toolbox for the core authoring of page objects, and if I do, that toolbox suddenly gets really big. So I'd definitely be open to a separate package with a whole bunch of helpers and whatnot, but am not seeing a good argument for putting text in the core. Anyway, sounds like you've already said you're fine with that.

But I also do want to expose the primitives that would enable users to implement things like assertExists, so how about we:

  1. Toss text
  2. Keep assertExists
  3. Implement getDescription(obj: PageObject): string (and use it from assertExists)
  4. Move WithElement to -private/types.ts and export it from index.ts so it's public (and use it from assertExists)

@NullVoxPopuli
Copy link
Contributor Author

changes made!

* A utility interface that can be unioned with a {@link PageObject}
* to denote that the `element` is _known_ to exist
*/
export interface WithElement {
Copy link
Owner

Choose a reason for hiding this comment

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

We already have a WithElement interface that's more targeted to PageObject and therefore IMO easier to use for its intended purpose. So let's update this one to look like that one, and then make array-proxy.ts import/use this instead of having its own.

/**
* Utility to get the fully resolved selector path of a {@link PageObject}
*/
export function getDescription(pageObject: PageObject): string {
Copy link
Contributor

@sukima sukima Feb 2, 2022

Choose a reason for hiding this comment

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

Can I just express my sincere and emphatic support of getDescription I've run into this many times and every time I come back disappointed that there is not a clean way to get the current selector string. The two main cases I've discovered so far are…

In debugging. Element not found is not very easy to discover where it is while selector '… … …' not found can really help someone staring at the Browser's console inspector to find where the typo is.

When using 3rd party test helpers. For example, ember-power-calendar has a calendarSelect() helper where the first param must be a string because it concatenates its own nested selectors on the one you passed. But as is now we can't convert a fractal-p-o selector back into the full selector from it's .element property.

Having this one escape hatch would make composing Page Objects not only easier to debug but also encourage the use of PageObjects like Martin Fowler originally envisioned by allowing developers to design a fractal-p-o with better methods especially methods that hook into 3rd party test helpers.

class Foo extends PageObject {
  form = selector('[data-test-selector=my-form]', class extends PageObject {
    calendar = selector('ember-power-calendar', class extends PageObject {
      selectDate(date) {
        return calendarSelect(getDescription(this), date);
      }
    });
  });
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the feedback @sukima! Relatedly, if you haven't read emberjs/rfcs#726 yet, I'd love to hear any thoughts/feedback you have on it. I'm hoping to make a push to get it moving again soon...

Get that coverage back up to 100%!
@bendemboski bendemboski merged commit f377aec into bendemboski:main Feb 2, 2022
@bendemboski bendemboski added the enhancement New feature or request label Feb 2, 2022
@NullVoxPopuli NullVoxPopuli deleted the helper-utilities branch February 2, 2022 22:58
@bendemboski
Copy link
Owner

Released in v0.2.2 -- thanks @NullVoxPopuli!

@NullVoxPopuli
Copy link
Contributor Author

yay! thanks!

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

Successfully merging this pull request may close these issues.

3 participants