add match exact text support

[mum-never-proud]

Resolves #272

[coveralls]

Coverage increased (+0.03%) to 98.694% when pulling caf14ebe7ace130ddd6217e78479cb245a0f3f53 on mum-never-proud:add-match-exact-support into 5814ba4a82b672dd899b0c4dac25355be01b16a6 on san650:master.

[ro0gr]

Thanks for opening PR! I agree that the issue should be addressed.

There are few quick implementation related observations:

• I believe we should avoid increasing reliance on jquery in general. It'd be better if we can achieve that matchExact behavior by our own, w/o extra pseudo selector of jquery.
• it feels like the feature is only needed for clickOn(is it correct). Maybe it should not leak to a Selector options, but be a clickOn option only?

[mum-never-proud]

@ro0gr thanks for reviewing

agreed for the first point, even I thought the same but since this package already uses jQuery thought of going with the flow :P

here is a quick & rough implementation

https://jsfiddle.net/s1ywa0dz/1/

for point 2 don't u think its necessary elsewhere?

[ro0gr]

this package already uses jQuery

That's true. However, we intend to reduce dependency on jQuery or even remove it at some point. So if we can avoid new integration points, let's avoid it.

for point 2 don't u think its necessary elsewhere?

No. Sorry, my statement must be confusing.

I don't think we need exact or exactMatch somewhere else other than clickOn. If add that option as a targetFilter on a Selector, it would be exposed as a finder option for any other properties, which we should avoid. So, I think, we have to scope the feature inside clickOn property only.

---

One more point. that seems to be important. I'm currently working on initial refactoring and preparations for the v2. It significantly simplifies the properties, and clickOn specifically. I'm going to create a WIP PR with this work today. Maybe it makes sense to do the work on top of that work, to avoid unncessary conflicts and simplify feature implementation.

[mum-never-proud]

@ro0gr IDK whether it makes sense, I have faced a scenario with one of the packages we use at work. Since upgrading a larger codebase is a bit of pain in the a** but the support for a certain feature was provided only in the latest release of the package

since I recently started contributing to OS I am not sure how this works, but wouldn't it be great to support the older version as well? especially in the case where haven't yet release v2 and the fact that it may take time

[ro0gr]

the support for a certain feature was provided only in the latest release of the package

I'm not sure to which feature do you refer. But assuming you are talking about findElement/findElementWithAssert, which was deprecated and replaced by findMany/findOne in the 1.17, I think you are right.

Initially I thought, just providing codemods for finders would make migration easy enough. But if consider a large codebase, which has addons with their own page objects, I can see how such a migration can become painful. I think I'll make a step back in the beta branch and restore these 2 utils with deprecations prolongated until v3, but w/o multiple for now. Other than those 2 utils, in practice, I don't expect any serious difficulties for upgrade.

Coming back to where to implement the feature. I'm not opposed to add it to the v1. It'd just take some extra effort to make it a part of the v2, since all the actions are going to be re-written slightly. Though, it should not be a big issue, and if you need it soon - go for it.

[mum-never-proud]

@ro0gr + 1, so you are saying if I made necessary changes in this PR, we can merge?

edit: also are we planning to remove jQuery completely in v2?

[ro0gr]

you are saying if I made necessary changes in this PR, we can merge?

yes.

are we planning to remove jQuery completely in v2?

There is no such a strict plan. I'd say it would be nice to have an ability to opt-out jQuery in scope of v2 on user permise, in order to avoid a hard dependency on jQuery selectors API.

[mum-never-proud]

I have fixed the exact option leak

but for a custom implementation of the pseudo selector, I guess we can go with this approach https://jsfiddle.net/s1ywa0dz/1/

before starting

$(selector, container) {
    if (container) {
      return $(selector, container);
    } else {
      // @todo: we should fixed usage of private `_element`
      // after https://github.com/emberjs/ember-test-helpers/issues/184 is resolved
      let testsContainer = this.testContext ?
        this.testContext._element :
        '#ember-testing';

      return $(selector, testsContainer);
    }
  }

my proposal is to check the selector with regex /[^:][a-z]*(?=\()/ig that extracts text between ::[a-z]( e.g button:containsExact("Hello") would extract containsExact we can have a set of custom helpers to do so but i am a bit worried that it will bloat the function

your thoughts?

cc @ro0gr

[ro0gr]

@mum-never-proud I'm not sure.

I'd like us to be able to add such a feature by changing only click-on-text action, w/o touching any other layers of the ember-cli-page-object codebase, and not involving custom jquery pseudo selectors. And I have to admit that's not currently feasible to do neither with v1 nor with v2.

I think ideally, we should be able to lookup an element to click in the action, and then pass it to the interaction method, so we don't enforce the rest of the system to know about the custom selectors just for one case scenario.

So, in my opinion, instead of (from v1): https://github.com/san650/ember-cli-page-object/blob/5814ba4a82b672dd899b0c4dac25355be01b16a6/addon-test-support/properties/click-on-text.js#L102 or (from v2-beta); https://github.com/san650/ember-cli-page-object/blob/8e20854371c00bd30cb873fa0c79605a1c73871f/addon-test-support/properties/click-on-text.js#L113

we need a way to pass the element instead of combination of pageObject, selector and options.

pseudo-code:

if (options.matchExact) {
  const els = findMany(this, selector, options).filter(el => $(el).text() === textToClick);
  guardMultiple(els);

  // also needs to handle contextual error somehow
  return click(els[0]);
}

smth like that still should be possible to achieve in v2(sorry for shifting to that direction), I'll take this use case into a consideration. But for v1 it doesn't seem possible or easy to achieve, cause the current execution contexts do not accept HTML elements to actions. Don't have a clear path forward right now, let me think a bit more about it please.

[mum-never-proud]

lately I was just brainstorming a bit on this approach

getCustomTextFilters it gets the custom filters to be applied, wanna make it generic so that it filters can further be applied in future like caseSensitive, caseInsensitive now these may sound dumb but for now only these came to my mind

from my understanding, we make use of assertElementExists in almost all actions in ember-cli-page-object

assertElementExists(selector, options) {
    /* global find */
    let result = find(selector, options.testContainer || findClosestValue(this.pageObjectNode, 'testContainer')).toArray();

    Object.values(getCustomTextFilters(options))
      .forEach(customFilter => {
        result = result.filter($ele => customFilter($ele.textContent.trim(), options.contains));
      });

    if (result.length === 0) {
      throwBetterError(
        this.pageObjectNode,
        options.pageObjectKey,
        ELEMENT_NOT_FOUND,
        { selector }
      );
    }

    return result;
  }

I think while asserting we must also return the element to reduce one round of querying dom, its my understanding that if we pass direct DOM Node jQuery doesn't query.

Object.values(getCustomTextFilters(options))
      .forEach(customFilter => {
        result = result.filter($ele => customFilter($ele.textContent.trim(), options.contains));
      });

this one pretty straight forward we get the user given custom text filters loop and filter out the element

the tests are passing and I don't see any regression happening, I am not sure if we are lacking tests

I think this general approach can be extended without depending on jQuery

still, I am not much familiarized with all of the codebase I might have missed a few edge cases.

let me know your thoughts or if you need further clarification :)

tanks!

cc: @ro0gr

[ro0gr]

we make use of assertElementExists in almost all actions in ember-cli-page-object

yes, but we aim to remove it in favour of findOne soon. see https://github.com/san650/ember-cli-page-object/pull/493/files#diff-64c0403b6dd8956dfe66fdde51ae86b5L94