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)

Signed-off-by: Zacharias Fragkiadakis <zacfragkiadakis@gmail.com>

- Refactors some tests in onboarding.spec.ts using `browser.waitUntil` to reduce flakiness - These assertions will now poll for the result instead of failing immediately - Simulates Radicle installation in the onboarding process by renaming the node home directory - Extracts common queries, actions, and assertions to helper files Signed-off-by: Zacharias Fragkiadakis <zacfragkiadakis@gmail.com>

Signed-off-by: Zacharias Fragkiadakis <zacfragkiadakis@gmail.com>

- 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 <zacfragkiadakis@gmail.com>

- 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 <zacfragkiadakis@gmail.com>

- Sets "waitFor" timeout globally - Extracts some functions to helper files - Generally cleans up the test suite Signed-off-by: Zacharias Fragkiadakis <zacfragkiadakis@gmail.com>

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?

maninak · 2025-01-08T01:13:30Z

test/e2e/wdio.conf.ts

+  specs: [
+    ['./specs/onboarding.spec.ts', './specs/settings.spec.ts'],
+    './specs/**/!(onboarding|settings).spec.ts',
+  ],


I roughly sketched this for you to consider if we could/should use in order to have a single point of definition for those specs that need to be in specific order and also help not forget to keep those array entries in sync as specs get added/removed.

/** * Add to this list any paths to spec files requiring to run in a specific order. */ const orderedSpecPaths = ['./specs/onboarding.spec.ts', './specs/settings.spec.ts'] const specsToExcludeFromGlob = orderedSpecPaths .map(specPath => specPath.split('/').pop()?.replace('.spec.ts', '')) ;[ orderedSpecPaths, `./specs/**/!(${specsToExcludeFromGlob.join('|')}).spec.ts`, ]

Feel free to adjust as needed or outright skip if you think it would be best to keep it WET (instead of DRY but more engineered and restrictive).

Either way please ensure that we're using (here and everywhere else as needed) the OS-agnostic nodejs separator imported from node:path, so that our code is more likely to run on windows (or at worst have less tech debt when the need comes to officially support it).

maninak · 2025-01-08T01:23:30Z

test/e2e/specs/settings.spec.ts

+
+    // This functionality does not seem to work
+    // eslint-disable-next-line max-len
+    it.skip('recognizes if the directory is created *after* the setting is updated', async () => {


Suggested change

it.skip('recognizes if the directory is created *after* the setting is updated', async () => {

it.skip('recognizes if the directory is created *after* the setting is set', async () => {

Minor copy improvement. At least for my brain, makes the case description a tad easier to parse.

maninak · 2025-01-08T01:23:49Z

test/e2e/specs/settings.spec.ts

+
+    // This functionality does not seem to work
+    // eslint-disable-next-line max-len
+    it.skip('recognizes if the directory is created *after* the setting is updated', async () => {


Suggested change

it.skip('recognizes if the directory is created *after* the setting is updated', async () => {

it.skip('recognizes if the directory is created *after* the setting is updated', async () => {

Minor copy improvement. At least for my brain, makes the case description a tad easier to parse.

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?

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 6 times, most recently from 0cecdcf to 974327c Compare December 30, 2024 18:03

QZera added 3 commits December 31, 2024 11:18

test(e2e): write tests for more onboarding screens

3b0d52c

Signed-off-by: Zacharias Fragkiadakis <zacfragkiadakis@gmail.com>

refactor(e2e): rename and enhance standard sidebar views assertion

adf2a6d

Signed-off-by: Zacharias Fragkiadakis <zacfragkiadakis@gmail.com>

QZera force-pushed the feat/settings-e2e-tests branch 9 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 <zacfragkiadakis@gmail.com>

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 <zacfragkiadakis@gmail.com>

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

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

maninak Jan 8, 2025

maninak Jan 8, 2025

maninak Jan 8, 2025

	it.skip('recognizes if the directory is created after the setting is updated', async () => {
	it.skip('recognizes if the directory is created after the setting is set', async () => {

	// 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

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

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