Add cypress tests

[vedhav]

A draft on adding cypress tests for teal.slice

How to run?

1. Make sure node is installed.
2. Go to the tests directory and run npm i to install the npm dependencies.
3. You can run the test on all the module functions using the tests/test_e2e.R file. Make sure that the working directory is tests.
4. You can also check a module individually by running this command in the shell: npm run test-e2e

[gogonzo]

I don't think testing framework should affect teal code. Isn't this better to get element by id instead of adding cy-* class?

[vedhav]

Agree that the testing framework should not affect the core package code. However, in the future when some changes are made to the id or class they would break our tests, Adding the cy-* classes in the logic we test gives a signal to the developer modifying the class to think about fixing the tests too. If we detect the elements using id/class/data attributes Cypress recommends adding these cy_* to reduce flaky tests.
But ideally, cypress docs recommend implementing tests without any class/id etc by trying to find the elements by just visual indicators for example "Find the input element with label ADSL" but in our case, I was unable to figure out a way to implement tests without class selectors yet.
So, I'd say let's see if we can implement them without any class/id selection and fallback to this when we're sure it's not possible.

[gogonzo]

However, in the future when some changes are made to the id or class they would break our tests

Contrary, changing testing framework will leave some redundant class specification in the package.

Adding the cy-* classes in the logic we test gives a signal to the developer modifying the class to think about fixing the tests too

Fair point. On the other side any errors will be indicated by the cypress tests result.

Find the input element with label ADSL

We can use specific selectors to find the right elements. We will probably need to add class to the FilterState to distinct by the class of the filtered-variable (for numerics we have plot-slider, etc)

• xpath selector //*[starts-with(@id, 'teal-main_ui-filter_panel-active-') and contains(@id, '-$dataname-') and contains(@id, '-inputs-')]
• css selector [id^='teal-main_ui-filter_panel-active-'][id*='-$dataname-'][id*='-inputs-']

[gogonzo]

I don't like the fact id is duplicated with "cy_" prefix. I propose following convention: since we need to be aware that certain element is tested let's just add cy-testable class to the existing class (as a second class). Then selector for this like will be

Suggested change

	cy.get(".cy-active-summary-table tbody tr").as("activeSummary");
	cy.get(".cy-testable.teal_active_summary_filter_panel tbody tr").as("activeSummary");

[vedhav]

👍🏽 Let's try to find out if it's possible to implement the tests without any change to the classes in the package as we talked about. If unsuccessful, we can use this convention, I like it.

[gogonzo]

I respect the idea of having a distinction between what's testable and what is not so app developer won't change this casually (I hope at least). It will be good to write down this convention somewhere that "testable" means don't touch it ;)

Also, I think we need to review all html classes to follow some "template". They seem to be random

[vedhav]

Also, I think we need to review all html classes to follow some "template". They seem to be random

Agreed

[m7pr]

if testable is propagated in some good-practice guidelines then this should be ok

[gogonzo]

I'm thinking about parent-child naming convention for filters. In FilterState card can have a general class like filter-card, then RangeFilterState$ui_inputs will just have .numeric-filter-inputs.

Suggested change

	.get(".cy-numeric-selection-inputs input:first")
	.get(".filter-card > .numeric-filter > .cy-testable.filter-card input:first")

Let's sit down and design a complete wireframe for teal/teal.slice and let's think about classes/ids of each element

[gogonzo]

It should be a generic shiny app instead of teal. Testing could be distorted by teal. I suggest to use shiny app from vignette. Should also test the output if and how data gets filtererd?

[gogonzo]

Please update:

• .gitignore to not include cypress artifacts and node modules
• .Rbuildignore to not include cypress at all

[m7pr]

npm run test-e2e

Is ability to run node a blocker for potential future developers writing cypress tests? Are people from teams like SME be able to execute those?

I think we should not modify the class name, as tests will indicate something failed and then developer will realize that the change of the class id/name broke tests and this needs to be resolved on the test side. This is the same with regular R function names. If you change the function name then the test that assumed a specific function name will fail and then you will realize you need to edit tests. We do not prepend R function names with testthat_ to point that this function is covered by tests. What if at some point we have all classes covered by cypress - do we need to have cy- prefix then?

[m7pr]

So for cypress tests you assume that there is a specific class id/name, so that's why you put all those divs( around in R code. Then you need to have node and know JavaScript to write tests? Is that right? Is JS part of CoreDev regular skillset?

[vedhav]

Closing the PR as we chose to go with the shinytest2