test(e2e): settings tests #167

QZera · 2024-12-01T16:39:17Z

Prerequisite #162

Tests the "Path to Rad Binary" and "Path to Node Home" extension settings

Notes:

The tests that verify that the extension recognizes paths that are created after the setting is updated are implemented but marked to be skipped by the runner. As discussed, this functionality should work but doesn't at the moment
The "Path to Node Home" tests are skipped on the Linux CI, as there seems to be an issue where the extension does not correctly pick up the initial node home path (from the RAD_HOME env var)

- adds tests for the "path to rad binary" setting - adds cleanup logic to the onboarding suite - updates config to run onboarding and settings suites sequentially - updates e2e ci workflow to ensure both platforms run regardless of each other's outcome Signed-off-by: Zacharias Fragkiadakis <[email protected]>

- Adds test for the "path to node home" setting - Allows skipping tests based on platform by including `@skip{platform}CI` in the test name Signed-off-by: Zacharias Fragkiadakis <[email protected]>

- Sets "waitFor" timeout globally - Extracts some functions to helper files - Generally cleans up the test suite Signed-off-by: Zacharias Fragkiadakis <[email protected]>

maninak

EDIT: Oh w8, reviewed wrong PR. I'll get to this one next, soz!

maninak

see above

maninak

I still need to have a second look, particularly for the details of the logic in settings.spec.ts, but for now some early comments.

In addition to the inline ones, I want to address a couple more general ones:

regarding changing setting values, I wonder if we take into account (and if we should, if not) the debounce that seems to be between setting changing the input field's value and it "registering". Perhaps we could "blur" the focus instead of waiting which seems to trigger registering of the new value instantly.
if running (a subset, as previously discussed) of the e2e tests locally is now safe, i.e. won't impact local setup, we should revisit restoring the npm script test:e2e inside the top-level script "test".

maninak · 2025-01-08T00:41:37Z

test/e2e/wdio.conf.ts

@@ -45,13 +59,15 @@ export const config: Options.Testrunner = {
    },
  ],
  logLevel: 'warn',
-  waitforTimeout: 10000,
+  waitforTimeout: 20000,


What prompted this increase? Is it possible to apply it per case as needed instead?

test/e2e/wdio.conf.ts

test/e2e/specs/settings.spec.ts

maninak · 2025-01-08T01:32:59Z

test/e2e/helpers/assertions.ts

+import type { OutputView, Workbench } from 'wdio-vscode-service'
+
+/**
+ * Asserts the output view contains the expected text.


The idiomatic term for this piece of UI is "Output Panel". Best to use that term since the term View is semantically overloaded in the context of VS Code extension development and refers to a specific different entity, which may be misleading.

Myself, I often opt to use Title Case whenever I refer to such idiomatic terms to better hint at that attribute of theirs, though I mindfully often opt not to, when I'm trying to avoid unnecessary visual overload. It's a subtle thing, do as you prefer, just thought I'd mention it once. When in doubt it's better to not use any special casing as using it wrongly would actually make things worse.

maninak · 2025-01-08T01:35:27Z

test/e2e/helpers/assertions.ts

+  await browser.waitUntil(
+    async () => {
+      /**
+       * The text in the output console is split by newlines, which can be affected by the size


Similarly to previous comment, Console (Panel) is another different piece of UI. Best here to use the term Output Panel or Output Channel (depending on if you prefer pointing to the representational or the model part of the UX (semantics can get complex, I know)).

maninak · 2025-01-08T01:50:41Z

test/e2e/specs/settings.spec.ts

+      pathToRadBinarySetting = await settings.findSetting(
+        'Path To Rad Binary',
+        'Radicle',
+        'Advanced',
+      )


Is it possible to refer to each setting by its identifier instead? It would make for less brittle referencing. Ids are defined in package.json, e.g. here it would be radicle.advanced.pathToRadBinary.

If you use the above in the settings search you'll get the single expected result

If you'd want to go all in you could even search for @id:radicle.advanced.pathToRadBinary, but I fail to see how prepending the @id makes any material difference.

Same for all instances of this.

maninak · 2025-01-08T02:30:57Z

test/e2e/specs/settings.spec.ts

+      await $`rm -rf ${tempNodeHomePath}`
+    })
+
+    // This functionality does not seem to work


When you find bugs while testing (which is great!!), it's best to create an issue instead, list the repro steps in there which by definition you know, and use a better comment here like e.g.:

Suggested change

// This functionality does not seem to work

// TODO: unskip when bug #xyz covered by this case is fixed

maninak · 2025-01-08T02:35:09Z

test/e2e/specs/settings.spec.ts

+  /**
+   * In Linux CI:
+   * - The extension is having issues resolving the correct node home directory (RAD_HOME).
+   * - Even when node home is set explicitly in the settings, the extension incorrectly reports it
+   *   as non-authenticated.
+   */
+  // eslint-disable-next-line max-len
+  describe('VS Code, when updating the "Path to Radicle to Node Home" setting, @skipLinuxCI', () => {


Skipping tests, especially on Linux CI, along with the whole setup that comes to support this on the runner level is both hacky and and risky.

These various odd issues we've been having on the environment level smell to me that we need to look upwards to sniff out the root cause (e.g. in e2e-testing.yaml, or wdio config, or investigate CLI bug with the maintainers, ...?) or it will keep forcing more hacks down our throats.

At the very least, here could we at least source the value of RAD_HOME alternatively e.g. with rad self --home and remove all those @skipLinuxCI markers and related supporting logic?

Signed-off-by: Zacharias Fragkiadakis <[email protected]>

QZera force-pushed the feat/settings-e2e-tests branch 11 times, most recently from 65d319d to 1046eb8 Compare December 1, 2024 19:08

QZera changed the title ~~Feat/settings e2e tests~~ test(e2e): setting tests Dec 1, 2024

QZera force-pushed the feat/settings-e2e-tests branch from 1046eb8 to 800e371 Compare December 2, 2024 07:07

QZera force-pushed the feat/settings-e2e-tests branch 17 times, most recently from 46b1f43 to 5a28f5e Compare December 27, 2024 10:43

QZera force-pushed the feat/settings-e2e-tests branch 13 times, most recently from fb82cd9 to a62b0b6 Compare January 4, 2025 10:23

QZera changed the title ~~test(e2e): setting tests~~ test(e2e): settings tests Jan 6, 2025

QZera force-pushed the feat/settings-e2e-tests branch from c29da1e to c1b8f16 Compare January 6, 2025 10:08

QZera added 3 commits January 6, 2025 12:35

test(e2e): write tests for "path to node home" setting

88d7fbe

- Adds test for the "path to node home" setting - Allows skipping tests based on platform by including `@skip{platform}CI` in the test name Signed-off-by: Zacharias Fragkiadakis <[email protected]>

refactor(e2e): clean up settings test suite

b79dae5

- Sets "waitFor" timeout globally - Extracts some functions to helper files - Generally cleans up the test suite Signed-off-by: Zacharias Fragkiadakis <[email protected]>

QZera force-pushed the feat/settings-e2e-tests branch from 747f07b to b79dae5 Compare January 6, 2025 10:35

QZera marked this pull request as ready for review January 6, 2025 10:39

QZera requested a review from maninak January 6, 2025 10:39

QZera self-assigned this Jan 6, 2025

maninak approved these changes Jan 7, 2025

View reviewed changes

maninak requested changes Jan 7, 2025

View reviewed changes

maninak requested changes Jan 8, 2025

View reviewed changes

docs(e2e): update terminology on expectOutputToContain

1e20b85

Signed-off-by: Zacharias Fragkiadakis <[email protected]>

QZera force-pushed the feat/settings-e2e-tests branch 2 times, most recently from a2e4e4f to 1e20b85 Compare January 22, 2025 09:33

QZera added 2 commits January 22, 2025 11:50

test(e2e): update test case titles

2b5d7a5

Signed-off-by: Zacharias Fragkiadakis <[email protected]>

refactor(e2e): dynamically generate spec order

9171a8e

Signed-off-by: Zacharias Fragkiadakis <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(e2e): settings tests #167

test(e2e): settings tests #167

QZera commented Dec 1, 2024 •

edited by maninak

Loading

maninak left a comment •

edited

Loading

maninak left a comment •

edited

Loading

maninak left a comment •

edited

Loading

maninak Jan 8, 2025

maninak Jan 8, 2025

maninak Jan 8, 2025

maninak Jan 8, 2025

maninak Jan 8, 2025

maninak Jan 8, 2025

	// This functionality does not seem to work
	// TODO: unskip when bug #xyz covered by this case is fixed

test(e2e): settings tests #167

Are you sure you want to change the base?

test(e2e): settings tests #167

Conversation

QZera commented Dec 1, 2024 • edited by maninak Loading

maninak left a comment • edited Loading

Choose a reason for hiding this comment

maninak left a comment • edited Loading

Choose a reason for hiding this comment

maninak left a comment • edited Loading

Choose a reason for hiding this comment

maninak Jan 8, 2025

Choose a reason for hiding this comment

maninak Jan 8, 2025

Choose a reason for hiding this comment

maninak Jan 8, 2025

Choose a reason for hiding this comment

maninak Jan 8, 2025

Choose a reason for hiding this comment

maninak Jan 8, 2025

Choose a reason for hiding this comment

maninak Jan 8, 2025

Choose a reason for hiding this comment

QZera commented Dec 1, 2024 •

edited by maninak

Loading

maninak left a comment •

edited

Loading

maninak left a comment •

edited

Loading

maninak left a comment •

edited

Loading