Bug: getByTestIdOrAriaLabel does not work data-testid

tracy-french commented 5 months ago

Which package(s) does this bug affect?

[ ] Create Plugin
[ ] Sign Plugin
[X] Plugin E2E

Package versions

0.18.0

What happened?

It's not possible to use getByTestIdOrAriaLabel with a data-testid. The method requires you to call it like getByTestIdOrAriaLabel('data-testid="my-id"') to use data-testid, but the method incorrectly creates the selector as [data-testid="data-testid="my-id""].

You can see the bug at https://github.com/grafana/plugin-tools/blob/8be2f927615c2e5a5d21a2b27398122dbe22bdfe/packages/plugin-e2e/src/models/pages/GrafanaPage.ts#L30.

What you expected to happen

The method should create the selector as [data-testid="my-id"].

How to reproduce it (as minimally and precisely as possible)

Use the method with a data-testid.

Environment

System:
    OS: macOS 13.6.4
    CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
    Memory: 352.62 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.19.0 - ~/.volta/tools/image/node/18.19.0/bin/node
    Yarn: 1.22.19 - ~/.volta/tools/image/yarn/1.22.19/bin/yarn
    npm: 10.2.3 - ~/.volta/tools/image/node/18.19.0/bin/npm
  Browsers:
    Chrome: 122.0.6261.129
    Edge: 122.0.2365.80
    Firefox: 123.0
    Firefox Developer Edition: 121.0
    Safari: 17.3
  npmPackages:
    @grafana/aws-sdk: 0.3.1 => 0.3.1
    @grafana/data: 10.2.0 => 10.2.0
    @grafana/e2e: 10.2.0 => 10.2.0
    @grafana/e2e-selectors: 10.2.0 => 10.2.0
    @grafana/eslint-config: ^6.0.0 => 6.0.1
    @grafana/experimental: 1.7.3 => 1.7.3
    @grafana/plugin-e2e: ^0.18.0 => 0.18.0
    @grafana/runtime: 10.2.0 => 10.2.0
    @grafana/tsconfig: ^1.2.0-rc1 => 1.2.0-rc1
    @grafana/ui: 10.2.0 => 10.2.0

Additional context

No response

sunker commented 5 months ago

Thanks for creating this issue @tracy-french!

The intent of this method is to provide a safe way to use grafana/e2e-selectors. These selectors were initially using aria-label attribute, but to improve accessibility most of them have been migrated to use data-testid instead. For legacy reasons, the selector value had to be prefixed with data-testid so that the resolver would know what attribute to look for. So the purpose of the getByTestIdOrAriaLabel method is to provide a safe way to find locators based on a Grafana e2e selector. You can find more information about this in the plugin-e2e docs.

If you have defined data-testid attributes to elements in your plugin, I suggest you use page.getByTestId to access them.

The method name getByTestIdOrAriaLabel is a bit misleading since there's more to it. I'll definitely add a more explanatory comment. I'm also considering changing the method name entirely to something like getByGrafanaSelector. Thoughts on that @mckn?

mckn commented 5 months ago

Thanks for creating this issue @tracy-french!

The intent of this method is to provide a safe way to use grafana/e2e-selectors. These selectors were initially using aria-label attribute, but to improve accessibility most of them have been migrated to use data-testid instead. For legacy reasons, the selector value had to be prefixed with data-testid so that the resolver would know what attribute to look for. So the purpose of the getByTestIdOrAriaLabel method is to provide a safe way to find locators based on a Grafana e2e selector. You can find more information about this in the plugin-e2e docs.

If you have defined data-testid attributes to elements in your plugin, I suggest you use page.getByTestId to access them.

The get that the method name getByTestIdOrAriaLabel is a bit misleading since there's more to it. I'll definitely add a more explanatory comment. I'm also considering changing the method name entirely to something like getByGrafanaSelector. Thoughts on that @mckn?

I think the getByGrafanaSelector is a better name for this method since it reflects a bit more what it does.

sunker commented 5 months ago

Great, I'll put up a PR that changes the name. We can mark getByTestIdOrAriaLabel as deprecated, but still support it until we release 1.0.0.

jackw commented 5 months ago

IMHO whilst the package is on a major zero I don't think there's a need to deprecate things, rather I'd vote to rip it out now. Major zero signifies that the package is in active development and breaking changes can and will happen. This allows us to remove any cruft as it evolves into the api we want to give to consumers without the risk of something deprecated arriving in the 1.0.0 release.

sunker commented 5 months ago

IMHO whilst the package is on a major zero I don't think there's a need to deprecate things, rather I'd vote to rip it out now. Major zero signifies that the package is in active development and breaking changes can and will happen. This allows us to remove any cruft as it evolves into the api we want to give to consumers without the risk of something deprecated arriving in the 1.0.0 release.

Yes let's do it! I'm updating the PR.

grafana / plugin-tools