Cypress helper function for stripping html

[lemmyadams]

Subject of the issue/enhancement/features

With more tests in place, there's some reused code that would be best placed in a custom cypress command to reduce the risk of errors. Such as:

function stripHtml(text) {
  const textWithoutHtml = text.replace(/<[^>]*>/g, '');
  cy.wrap(textWithoutHtml).as('text');
}

[oliverfoster]

function testQuestionButtons(buttonsObject = {btn__action: 'Submit', btn__feedback: 'Show feedback'}) {
  Object.keys(buttonsObject).forEach((key) => {
    cy.get(`.${key}`).should('contain', buttonsObject[key])
  })
}

This is much more generic than it looks:

function hashContains(hash) { // the name needs changing
  Object.entries(hash).forEach(([key, value]) => {
    cy.get(`.${key}`).should('contain', value)
  })
}

The default values should not be hardcoded in English but instead be passed from the loaded JSON.

Reference:

• Object.entries
• Destructuring assignment

[oliverfoster]

There are 1001 ways of doing the stripHtml function:

function stripHtml(text) {
  const textWithoutHtml = text.replace(/<[^>]*>/g, '');
  cy.wrap(textWithoutHtml).as('text');
}

As you can see here: https://stackoverflow.com/questions/822452/strip-html-tags-from-text-using-plain-javascript

What are you going to use this for?

[lemmyadams]

There are 1001 ways of doing the stripHtml function:

function stripHtml(text) {
  const textWithoutHtml = text.replace(/<[^>]*>/g, '');
  cy.wrap(textWithoutHtml).as('text');
}

As you can see here: https://stackoverflow.com/questions/822452/strip-html-tags-from-text-using-plain-javascript

What are you going to use this for?

I'm going to use it for stripping the html. On that link currently its the 2nd bit of code, it was just the simplest design which I chose. I can pick the first one though if thats preferred or any other option.

[lemmyadams]

function testQuestionButtons(buttonsObject = {btn__action: 'Submit', btn__feedback: 'Show feedback'}) {
  Object.keys(buttonsObject).forEach((key) => {
    cy.get(`.${key}`).should('contain', buttonsObject[key])
  })
}

This is much more generic than it looks:

function hashContains(hash) { // the name needs changing
  Object.entries(hash).forEach(([key, value]) => {
    cy.get(`.${key}`).should('contain', value)
  })
}

The default values should not be hardcoded in English but instead be passed from the loaded JSON.

Reference:

• Object.entries
• Destructuring assignment

My initial thought was to reduce duplicated code here, and so I was going to have a default param for the submit and feedback buttons. I can get the text values from the course I presume so I'll update that there. Whilst I suppose this is a very generic function, what would stop it from becoming a general testElementContains function? Which I feel like would add a lot of complexity to something that doesn't need to be complex.
Just trying to think of readability here which I think is overlooked a lot in the aim of getting overly technical, and I want to avoid setting the object manually in every single test if that object is going to be the exact same for all the question components at least

[oliverfoster]

It's not really a helper at all it's a very specific externalised feature test.

Helpers are more like stripHtml above, they're generic repeatable utilities or standardised logic and usually not full tests on a specific feature.

what would stop it from becoming a general testElementContains function

That's exactly what it should be really, to become a helper.

[lemmyadams]

It's not really a helper at all it's a very specific externalised feature test.

Helpers are more like stripHtml above, they're generic repeatable utilities or standardised logic and usually not full tests on a specific feature.

what would stop it from becoming a general testElementContains function

That's exactly what it should be really, to become a helper.

Okay then lets pretend its not a helper function per se, where should that live in the code structure?

[oliverfoster]

If you want to externalise a test and reuse it across plugins, we don't have a mechanism for doing that, you'd have to repeat the 5 lines of code in each plugin, either in an imported file (if that's possible) or at the top of each test file.

You've put a common test into core the framework, which is used only by question components, I'd assume?

I guess that kind of is the right place to put it... maybe

You could have your testElementsContain function as a helper/command and then in the test file:

cy.testElementsContain({ 
  '.btn__action': textInputComponent._buttons._submit.buttonText || this.data.course._buttons._submit.buttonText, 
  '.btn__feedback': textInputComponent._buttons._showFeedback.buttonText || this.data.course._buttons._showFeedback.buttonText
})

I want to avoid setting the object manually in every single test if that object is going to be the exact same for all the question components at least

I don't think you should avoid setting the object manually. It makes it very clear what's being tested. Or just don't test the buttons here, test them in the core where the code lives? (obviously that's slightly difficult as there are not components in the core to test against)
Note: Buttons are also overridable from the component config

testElementsContain : You could add the not exists idea from the other helper also, maybe?

Otherwise, core would be the best place to put the question buttons externalised test, as that's where all of the other code for question buttons lives. We'd have to find a way to import a core/test/e2e/helpers file, I'd assume, which could get quite messy.

[oliverfoster]

I'm going to use it for stripping the html. On that link currently its the 2nd bit of code, it was just the simplest design which I chose. I can pick the first one though if thats preferred or any other option.

I see now, yes, thanks for testing the should('contain', text) function. It makes more sense. The document.createElement way, that you've changed to, is quite a bit better than the regex, as it parses the text as html and then uses the browser apis to derive the text. The regex one is prone to all kinds of oddities, special cases, poorly formed html etc.

Good choice.

[lemmyadams]

I don't think you should avoid setting the object manually. It makes it very clear what's being tested. Or just don't test the buttons here, test them in the core where the code lives? (obviously that's slightly difficult as there are not components in the core to test against) Note: Buttons are also overridable from the component config

I find it confusing that sometimes we're in favour of making what is to be tested clear, like hardcoding an object, but not other times. It seems as though the button situation is far more complicated than I understood so its best to just remove it for now

[oliverfoster]

I find it confusing that sometimes we're in favour of making what is to be tested clear... but not other times.

Can you give the line references and examples of where we've chosen to not make things clear? We can set about correcting them.

like hardcoding an object,

I don't understand, do you mean the English default function parameters?

I'm always available to chat on Google meet, element/gitter or teams if you want? I think we've only spoken through text so far.

[lemmyadams]

Can you give the line references and examples of where we've chosen to not make things clear? We can set about correcting them.

I don't understand, do you mean the English default function parameters?

Creating the same object for at least 5 different tests doesn't make any sense to me as we should want to reduce repeated code as much as possible. It's no different from a testContainsOrNotExists, so I don't understand why one is fine but the other is a terrible idea. Changing it to a very vague function is good for usability, but that extra use cases don't exist right now and it makes things much more confusing. Changing it to a hashContains would really confuse me

[oliverfoster]

Creating the same object for at least 5 different tests doesn't make any sense to me as we should want to reduce repeated code as much as possible.

I agree about the repetition being suboptimal. However, tests should live in the same repo as the code they're testing. You've put a test for the adapt-contrib-core question buttons view in adapt_framework, the compilation/development/translations suite.
We segregate the code into responsibilities and into separate repos so that it's easy to find and so that each section is as loosely coupled and as replaceable as possible. I'm afraid code colocation trumps reducing repetition, always. Otherwise we end up with things in places that are hard to find. There are a few exceptions (vanilla and spoor), but as we have a choice, I suggest we don't make that mistake again. Realistically we need repeatable tests, for the question buttons, stored in the core, that can be reused by each question component. We can't do that at the moment, so let's not do it yet. There are plenty of other tests that can be written.

It's no different from a testContainsOrNotExists, so I don't understand why one is fine but the other is a terrible idea.

One is a test with a specific purpose, name (testQuestionButtons) and defaults that lives in the wrong place. The other (testElementsContain) is a generic utility function that lives in the right place, which can be utilised to perform a test, defined and located appropriately, in each question component. As you've acknowledged, question buttons is much more complicated than it appears.

hashContains

I agree the name is terrible, I did comment to say that the name needed changing, I did suggest and accept alternatives, the name wasn't the point, the colocation and misplacement was the point. The generic function was my effort to help in the reduction of repetition. I should have just said no to the placement of the test.

Apologies that I didn't make any of that clear.