test: happy path playwright query tests #290

tracy-french · 2024-03-12T16:14:18Z

What this PR does / why we need it: This change includes happy path playwright query tests for all seven queries exposed by the query editor. The work includes:

Happy path test cases for each query using mocked data and making assertions on the rendering of expected form elements.
Addition of QueryEditor page object model test utility used to reduce duplication within tests and make it easier to write and maintain tests.
Addition of queryEditor test fixture to remove the need to instantiate the QueryEditor object model explicitly in every test.
Addition of missing ARIA labels in query editor components used for testing selectors.

Next steps:

Tests utilizing all of the available form fields (e.g., resolution, aggregation, time, format, etc.).
Tests for failure cases.
Improved request interception code (there is current significant duplication and code size).

sarahzinger

Cool stuff! Thanks for contributing again! I'll let Kevin take another look in case I'm missing anything. I've shared a few gut reactions on code-style but I'm not that experienced with playwright so I'm not confident they are useful opinions or not. Might be worth sticking with what's here and seeing how it goes? Will defer to @kevinwcyu for a final approval/thoughts.

sarahzinger · 2024-03-12T18:53:17Z

src/components/query/ListAssetsQueryEditor.tsx

@@ -102,6 +102,7 @@ export class ListAssetsQueryEditor extends PureComponent<Props, State> {
        <div className="gf-form">
          <InlineField label="Model ID" labelWidth={firstLabelWith} grow={true}>
            <Select
+              aria-label="Model ID"


would it make sense to use data-testid instead of an aria label if the sole purpose is for testing? I think we want to only add aria labels it we are trying to fix an accessibility issue.

I also see we have the option to link the inlinefield label: grafana/grafana#31230

Not sure if that works with the new e2e package but might be worth looking into

These ARIA labels are actually already used with the new style of components, but it seems we forgot to add them to both places in multiple instances. For example, see https://github.com/grafana/iot-sitewise-datasource/blob/main/src/components/query/ListAssetsQueryEditor.tsx#L74 for the model ID label.

Thanks @sarahzinger, that's a good call out.

I found some additional docs around testing that suggest using htmlFor on the label component, <InlineField /> in this case, and using inputId on the <Select /> component.

I tried it out and the label does have the correct association to the input element when using htmlFor and inputId. The getByLabel locator can be used to click on the <Select />.

Sorry @tracy-french, I suggested using the aria-label along with the getByTestIdOrAriaLabel function for the form fields in your previous E2E PR, but I think Sarah's suggestion is a better one for us to follow since an accessible label already exists and can be connected with htmlFor and inputId.

Updated all of the locators to use page.getByLabel and updated the JSX to use inputId where needed.

sarahzinger · 2024-03-12T19:16:09Z

tests/helpers.ts

+  },
+});
+
+export { test, expect };


hmmm.... Maybe this is a personal style preference and a lack of familiarity with playwright conventions but I am not sure this abstraction is easier to read/write tests with? It definitely dries up the code, but if a test does fail or you need to write your own test you now need to know to look in a helper file to find this fixture and then look at queryeditorpage and then there might be a worry that a small change in one could affect other tests?

I guess when it comes to tests I almost don't mind a bit of redundancy if it makes them easier to read. But I don't want to block for a personal preference.

Idk what do you think @kevinwcyu in your research does this kind of best practice for playwright you think? I'm happy to go with whatever you all think makes the most sense.

Definitely, makes sense. I'm a fan of using fixtures, but this is indeed a very simple fixture and does not currently add much value beyond preventing you from having to instantiate the page object model.

As the test cases are expanded, something which may add a lot of value are test fixtures which reduce setup boilerplate when we're trying to test a particular state of the query editor multiple times. For instance, a getPropertyValueQueryEditor providing a query editor with the "Get property value" query pre-selected and exposing a page object model with the locators for form elements specific to only that query (i.e., no resolution/aggregation/etc.). Or even more valuable would be a fixture with an asset property (or multiple) already selected so that you can perform a series a tests without explicitly going through the flow of selecting an asset and then selecting a property in every test.

But those more valuable features don't need what I did in this PR now lol. If we want to wait until the value is clear, I am fine removing the fixture from this change. :)

I think this structure provides a good foundation for code standard/style 👍 We might not see the value on the current scale but doesn't hurt to keep it

I think it's useful to include this for interacting with the different select elements in the query editor, especially for the select elements that load options asynchronously. I'd be happy to keep it in since it does follow Playwright conventions.

Ok cool, let's give it a go, and see how it works out for us!

tracy-french · 2024-03-12T21:14:09Z

@sarahzinger Thank you for reviewing! :)

chejimmy

LGTM! Lay down an excellent foundation and setting the code style for tests

chejimmy · 2024-03-12T21:46:57Z

tests/helpers.ts

+  },
+});
+
+export { test, expect };


I think this structure provides a good foundation for code standard/style 👍 We might not see the value on the current scale but doesn't hurt to keep it

kevinwcyu

Nice, the page object model for the query editor looks really good. Just a few minor suggestions.

kevinwcyu · 2024-03-13T19:46:15Z

src/components/query/ListAssetsQueryEditor.tsx

@@ -102,6 +102,7 @@ export class ListAssetsQueryEditor extends PureComponent<Props, State> {
        <div className="gf-form">
          <InlineField label="Model ID" labelWidth={firstLabelWith} grow={true}>
            <Select
+              aria-label="Model ID"


Thanks @sarahzinger, that's a good call out.

I found some additional docs around testing that suggest using htmlFor on the label component, <InlineField /> in this case, and using inputId on the <Select /> component.

I tried it out and the label does have the correct association to the input element when using htmlFor and inputId. The getByLabel locator can be used to click on the <Select />.

Sorry @tracy-french, I suggested using the aria-label along with the getByTestIdOrAriaLabel function for the form fields in your previous E2E PR, but I think Sarah's suggestion is a better one for us to follow since an accessible label already exists and can be connected with htmlFor and inputId.

kevinwcyu · 2024-03-13T20:28:49Z

tests/helpers.ts

+  },
+});
+
+export { test, expect };


I think it's useful to include this for interacting with the different select elements in the query editor, especially for the select elements that load options asynchronously. I'd be happy to keep it in since it does follow Playwright conventions.

kevinwcyu · 2024-03-13T20:43:54Z

tests/queryEditor.page.ts

+ * Query Editor page object model testing utility providing properties and
+ * methods for common locators and actions.
+ */
+export class QueryEditorPage {


Suggested change

export class QueryEditorPage {

export class QueryEditor {

Minor nit: can this be named QueryEditor since it's a component within the PanelEditPage. It would be good to name this file queryEditor.model.ts along with this change.

tracy-french marked this pull request as ready for review March 12, 2024 16:22

tracy-french requested a review from a team as a code owner March 12, 2024 16:22

tracy-french requested review from sarahzinger, idastambuk and kevinwcyu and removed request for a team March 12, 2024 16:22

sarahzinger reviewed Mar 12, 2024

View reviewed changes

chejimmy approved these changes Mar 12, 2024

View reviewed changes

kevinwcyu reviewed Mar 13, 2024

View reviewed changes

test: happy path playwright query tests

f64f4e4

tracy-french force-pushed the test-happy-path-queries branch from 127ea79 to f64f4e4 Compare March 14, 2024 21:53

tracy-french requested review from sarahzinger and kevinwcyu March 14, 2024 22:07

sarahzinger approved these changes Mar 15, 2024

View reviewed changes

sarahzinger merged commit f1ab5bc into main Mar 15, 2024
12 checks passed

sarahzinger deleted the test-happy-path-queries branch March 15, 2024 13:43

sarahzinger added the effort/small label Mar 15, 2024

idastambuk mentioned this pull request Apr 4, 2024

Prepare 1.18.0 #303

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: happy path playwright query tests #290

test: happy path playwright query tests #290

tracy-french commented Mar 12, 2024 •

edited

Loading

sarahzinger left a comment

sarahzinger Mar 12, 2024

tracy-french Mar 12, 2024

kevinwcyu Mar 13, 2024

tracy-french Mar 14, 2024

sarahzinger Mar 12, 2024

tracy-french Mar 12, 2024

chejimmy Mar 12, 2024

kevinwcyu Mar 13, 2024

sarahzinger Mar 15, 2024

tracy-french commented Mar 12, 2024

chejimmy left a comment

chejimmy Mar 12, 2024

kevinwcyu left a comment

kevinwcyu Mar 13, 2024

kevinwcyu Mar 13, 2024

kevinwcyu Mar 13, 2024

tracy-french Mar 14, 2024

test: happy path playwright query tests #290

test: happy path playwright query tests #290

Conversation

tracy-french commented Mar 12, 2024 • edited Loading

sarahzinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tracy-french commented Mar 12, 2024

chejimmy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinwcyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tracy-french commented Mar 12, 2024 •

edited

Loading