test(featurelist): Cypress tests for featurelist

[bradh]

This adds cypress tests for the new features display. However I'd appreciate feedback on whether there is a cleaner way. In particular, I'm not pleased with doing the selector on icons.

Is it acceptable to modify code (e.g. to add a stable id) to make testing more reliable?

[justin-bits]

1. Move the test to new folder ‘regression-tests/layers-dialog’. The smoke-test folder should only contain tests for much larger functionality and this is just a single dialog.
2. Rename the test to layer-features.spec.js for consistency with the terminology used in the application and something that isn't tied to the implementation (slick-grid).
3. All selectors used in tests should be human readable in the spec and come from selectors.js. This extension can be helpful in building selectors for an element: https://chrome.google.com/webstore/detail/css-selector-helper-for-c/gddgceinofapfodcekopkjjelkbjodin
4. Create fixtures/regression-tests/layers-dialog/layer-features and add a new and unique test file there. Sharing data across tests will increase maintenance overhead as changes to any data file could impact multiple tests. When possible use unique data for each test. More variety in testing data will better exercise the application.
5. The title on the dialog should have been Layer Features or something equivalent instead of the name of the file. That would be consistent with other similar dialogs like Layer Description, Rename Layer, Export Data, etc. It could then be used as part of the selector. Or, an id=“layerFeatures” could be used as most dialogs have an id. However, the selector data-testid="featurelist” works just fine.
6. Tests missing for Sort Selected (context), Export, Go To, Show Selected Only checkbox, height slider, ctrl+click selections, shift+click selections, multiple instances of the dialog with different layers, sort via clicking on header, moving columns via drag, Show Description (for which test data currently in use in inadequate). Isolate different tests within the spec with an it for each area/category.
7. Consider smoke tests of this capability for other types of data in a new test spec.
8. Drop the use of .within and make selectors more specific instead. Using .within leads to lots of nesting and a less readable test.

Change this:

cy.get('[data-testid=\'featurelist\']').within(function() {
    cy.get('[ng-if=\'ctrl.status\']').should('be.visible');
    cy.get('[ng-if=\'ctrl.status\']').should('contain', '291 records');
});

To this:

cy.get(os.layerFeaturesDialog.STATUS_BAR)
        .should('be.visible')
        .should('contain', '291 records');

…with os.layerFeaturesDialog.STATUS_BAR being [data-testid=\'featurelist\'] [ng-if=\'ctrl.status\'] set somewhere in selectors.js.

[bradh]

I need clarification on 5. - are you just commenting on the UI / implementation, or asking for a change in the test?

So are you asking for changes to #561 implementation? If not, the title is what it is. There is an id, but its not fixed.

<div class="js-window modal d-flex c-window o-window ng-isolate-scope ui-draggable ui-resizable active" ng-class="{'active': active}" id="featureList-kml-12r235zciy72" label="qppairportols.kml" icon="fa fa-table" x="center" y="center" width="800" min-width="600" max-width="2000" height="600" min-height="400" max-height="2000" modal="false" show-close="true" style="width: 800px; height: 500px; left: 368px; top: 0px; z-index: 990;">

I think I can deal with the other issues (except maybe 6 and 7 which is a lot of additional test scope), but I'm not sure how to deal with 5.

[justin-bits]

5. I'm just noting that this dialog differs from others. Since this PR was just for the tests, no action.
   6/7. How thorough the GUI tests are / what levels of effort should be put in by developers probably requires a larger discussion.

[bradh]

I'm not going to do all of your tests. If that is unacceptable, say now and I'll stop wasting everyone's time.

If you're willing to take the tests I'm willing to write (on my own time), and a ticket that identifies the additional tests (6/7), then we have a way forward.

[justin-bits]

@bradh, apologies I didn't realize you were an open source contributor. We can make a note to expand coverage in the future for the test you wrote. Thank you for our efforts.

[schmidtk]

I would scratch out 5 entirely. The window title uses the title of the layer because a feature list can be opened for each layer. This helps the user distinguish which data they're looking at, and should not be changed to something generic.

As far as id's go, any window that can have multiple instances should not have a generic id. HTML id attributes should be unique in the DOM.

[schmidtk]

In terms of selectors, the use of within is preferable because it locates the top-level element (the feature grid) once and scopes following selectors from there. This is both more efficient (working from a subset of the DOM, vs starting from the root each time), and allows following selectors to be both shorter (faster to locate) and less specific without risk of matching other elements in the DOM.

I'm also a bit conflicted on selectors.js. I'm heavily in favor of creating enums for reuse and ease of updating if the DOM changes, but we should be maintaining those in a more organized manner. A single file with every test selector is not easy to maintain.

[justin-bits]

@bradh, after discussion with @schmidtk, I've revised my original comment.

Updated:

1. Move the test to new folder regression-tests/layers-dialog.
2. Rename the test to layer-features.spec.js
3. Use enums from selectors.js
4. Create fixtures/regression-tests/layers-dialog/layer-features and add a new and unique test file there. Be sure to use a test file that will support testing of the Show Description dialog.
5. Strike
6. Include tests for the Show Description dialog. For the remainder, see Expand test coverage for Feature List dialog #581
7. Expand test coverage for Feature List dialog #581
8. Strike, see Refactor Cypress selectors #580

To clarify on the new regression-tests folder... All existing tests have been smoke tests thus far - quick tests of large functionality. Those are generally finished. New tests created going forward will be part of full regression testing. These tests will be more thorough/detailed and directed at smaller portions of functionality. layer-features.spec.js just happens to be the first one. The new folder structure will help to keep the tests organized and make the distinction clear.

[bradh]

I've done 1 and 2.

I've done 4 except for the part about the description - its commented out in the KML, because it causes unexpected breakage. See below.

For 3. I've refactored out the selectors, but putting them into a different file in a different directory seems wrong. If they aren't actually shared between test files, then it makes more sense to keep the selectors and usage together.

For 5, 7 and 8 I took no action.

For 6, introducing even a trivial description makes cypress error. It appears that the canvas is breaking the visibility / interaction.

  1) Feature grid Shows data:
     CypressError: Timed out retrying: expected '[ <div.ui-widget-content.slick-row.even>, 6 more... ]' to be 'visible'

This element '[ <div.ui-widget-content.slick-row.even>, 6 more... ]' is not visible because it has CSS property: 'position: fixed' and its being covered by another element:

<canvas class="ol-unselectable" width="1279" height="920" style="width: 1279px; height: 920px;"></canvas>
      at Object.cypressErr (http://localhost:8282/__cypress/runner/cypress_runner.js:83220:11)
      at Object.throwErr (http://localhost:8282/__cypress/runner/cypress_runner.js:83185:18)
      at Object.throwErrByPath (http://localhost:8282/__cypress/runner/cypress_runner.js:83212:17)
      at retry (http://localhost:8282/__cypress/runner/cypress_runner.js:76640:16)
      at http://localhost:8282/__cypress/runner/cypress_runner.js:68612:18
      at tryCatcher (http://localhost:8282/__cypress/runner/cypress_runner.js:132142:23)
      at Promise._settlePromiseFromHandler (http://localhost:8282/__cypress/runner/cypress_runner.js:130160:31)
      at Promise._settlePromise (http://localhost:8282/__cypress/runner/cypress_runner.js:130217:18)
      at Promise._settlePromise0 (http://localhost:8282/__cypress/runner/cypress_runner.js:130262:10)
      at Promise._settlePromises (http://localhost:8282/__cypress/runner/cypress_runner.js:130337:18)
      at Async._drainQueue (http://localhost:8282/__cypress/runner/cypress_runner.js:127066:16)
      at Async._drainQueues (http://localhost:8282/__cypress/runner/cypress_runner.js:127076:10)
      at Async.drainQueues (http://localhost:8282/__cypress/runner/cypress_runner.js:126950:14)
      at <anonymous>

[bradh]

Also could consider whether https://github.com/ngageoint/opensphere/blob/master/cypress/integration/smoke-tests/generate-heatmap.spec.js is really a "smoke test".

[justin-bits]

• The selectors do need to be moved to selectors.js. There will be other tests that use those in the future. There are no tests that currently use selectors stored anywhere other than selectors.js.
• All selectors should be enums. Change lines like this accordingly y.get('[title=\'' + TEST_FIXTURE_DATA + '\']').should('be.visible');.
• There’s redundancy with several selectors also using [data-testid=\'featurelist\’]. The use of within should negate the need for this redundancy.
• The test failed due to cy.get(os.layerFeaturesDialog.ROWS).should('be.visible’); not being wrapped in a within. It wasn't due to the Description column.
• The test needs to open the Description dialog, to verify it opens and that the content is as expected.
• generate-heatmap.spec.js is definitely a smoke test. A full regression test would need heat maps generated from multiple types of geometry from multiple files with all style options verified, export tested, etc.

[schmidtk]

On the point of selectors, we're planning on organizing selectors.js into multiple files (see #580) because it's already getting a bit out of control. I'm open to keeping selectors in the file they're used if it's not expected they'll be reused by other tests. That makes them very easy to find and maintain, and a specific dialog like the feature list may be a good example of that. Dialogs like Layers are going to be used by a lot of tests, so their selectors should be accessible to all tests.

[bradh]

Please understand that within limits the DOM scope. Anything in a different branch of the DOM tree (like the context menu) won't be found.

[schmidtk]

The visibility error may be related to cypress-io/cypress/issues/2558 or cypress-io/cypress/issues/1379. I'm not seeing a current workaround in either of those issues.

[justin-bits]

Changing the position worked for me when checking visibility.

cy.get(os.layerFeaturesDialog.ROWS_CELLS)
        .invoke('css', 'position', 'static')
        .should('be.visible')
        .invoke('css', 'position', 'absolute');

Using .click({force: true}) worked partially for clicking. Unfortunately the app isn't seeing the click and fails on verifying 7 records (1 selected). I thought this part was working at one point? Manually triggering some mouse events might be needed here.

[bradh]

Using .click({force: true}) worked partially for clicking. Unfortunately the app isn't seeing the click and fails on verifying 7 records (1 selected). I thought this part was working at one point? Manually triggering some mouse events might be needed here.

It all passed for me (and Travis) without the description entry in the data. That might be an artifact of the timing on a VM, or some other difference.

[schmidtk]

It does seem to be a timing issue. Even with the description tests disabled, I get the same visibility errors Brad was seeing with them enabled.

[bradh]

I've taken a different approach to the row selection (choosing a cell instead), which works OK. That was based on @justin-bits position idea and the discussion in the cypress-io tickets that suggested that visible means "all visible".

That allowed me to test the description capability.

[schmidtk]

Ah I see, that's good to know. Thanks for working through this!