Fix failing to start shell being hidden from user #5626

Nokel81 · 2022-06-14T16:21:57Z

Signed-off-by: Sebastian Malton sebastian@malton.name

fixes #5513

github-actions · 2022-10-05T12:12:17Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2022-10-05T20:46:30Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions · 2022-10-05T20:46:46Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions · 2022-10-13T11:52:03Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

jansav · 2023-01-11T06:26:58Z

src/main/start-main-application/runnables/setup-lens-proxy.injectable.ts

@@ -82,7 +82,7 @@ const setupLensProxyInjectable = getInjectable({
    };
  },

-  causesSideEffects: true,
+  // causesSideEffects: true,


Remove line

jansav · 2023-01-11T06:29:53Z

src/renderer/components/dock/terminal/store.ts

+      this.connections.set(tab.id, { api, terminal });
+    });
+
+    const connectionPromise = api.connect();


Why are we not awaiting for promise?

So that the users of connect can be sync for when they want to do things that are not related to the connection being fully active.

jansav · 2023-01-11T06:34:38Z

src/common/utils/channel/channel.test.ts

+import type { RequestFromChannel } from "../../../renderer/utils/channel/request-from-channel.injectable";
+import requestFromChannelInjectable from "../../../renderer/utils/channel/request-from-channel.injectable";


We shouldn't be removing the injection token. requestFromChannel is contract which is implemented separately in renderer and main but the contract is same.

Important thing is that channel is something that can be called from anywhere and it doesn't know who answers. That being said, the "request" or "message" doesn't even have to go over IPC, the listener and caller can be located in same environment. (It doesn't work like this yet due YAGNI but conceptually that's what Channel is.)

The contract isn't the same. A request channel being requested on has no meaning on main and thus main shouldn't even know about it.

main can request stuff from channel. It just doesn't know who answers it, might be main or renderer.

No it cannot, where is that possible?

jansav · 2023-01-11T06:39:12Z

src/renderer/utils/sync-box/provide-initial-values-for-sync-boxes.injectable.ts


 const provideInitialValuesForSyncBoxesInjectable = getInjectable({
  id: "provide-initial-values-for-sync-boxes",

  instantiate: (di) => ({
    id: "provide-initial-values-for-sync-boxes",
    run: async () => {
-      const requestFromChannel = di.inject(requestFromChannelInjectionToken);
+      const requestFromChannel = di.inject(requestFromChannelInjectable);


Revert, see above discussion about the requirement for injectionToken.

jansav · 2023-01-11T06:41:51Z

src/common/app-paths/app-paths.test.ts

+  afterEach(() => {
+    builder.quit();
+  });
+


No, this implies that there is some shared global state in the builder. We need to figure out what that is and remove it. There shouldn't be any requirement for cleanups after test.

jansav · 2023-01-11T06:47:18Z

src/renderer/components/test-utils/get-application-builder.tsx

@@ -703,6 +706,13 @@ export const getApplicationBuilder = () => {
      await startApplication({ shouldStartHidden: true });
    },

+    quit() {


This isn't correct way to quit the application. This should use all same code than we use when application is quit in real life. Extract the function used in production and use it here.

quit shouldn't be part of all behavioural tests. There should be explicit behavioural tests for quitting the application and rest of them shouldn't know anything about it. That being said, afterEach requirement is not welcome.

jansav · 2023-01-11T06:50:18Z

package.json

@@ -449,7 +449,7 @@
    "webpack-cli": "^4.9.2",
    "webpack-dev-server": "^4.11.1",
    "webpack-node-externals": "^3.0.0",
-    "xterm": "^4.19.0",
+    "xterm": "^5.0.0",


This commit removes the last usage for earlier introduce image-snapshot dependency. Could you remove it?

jansav · 2023-01-11T06:51:54Z

package.json

@@ -450,27 +449,15 @@
    "webpack-dev-server": "^4.11.1",
    "webpack-node-externals": "^3.0.0",
    "xterm": "^5.0.0",
-    "xterm-addon-fit": "^0.5.0"
+    "xterm-addon-fit": "^0.5.0",


Why is this commit part of this monster PR at all? Doesn't have anything to do with fixing something, it's adding stuff.

jansav · 2023-01-11T06:59:09Z

src/main/lens-proxy/proxy-functions/shell-api-request.injectable.ts

@@ -27,14 +27,21 @@ const shellApiRequestInjectable = getInjectable({
      const nodeName = searchParams.get("node");
      const shellToken = searchParams.get("shellToken");

+      console.log("got shell api request", { tabId, clusterId: cluster?.id, nodeName });


Switch to using logger.

jansav · 2023-01-11T07:04:31Z

package.json

-    "create-release-pr": "node ./scripts/create-release-pr.mjs"
+    "create-release-pr": "node ./scripts/create-release-pr.mjs",
+    "prefix-canvas-deps": "npx swc ./scripts/fix-canvas-deps.ts -o ./scripts/fix-canvas-deps.mjs",
+    "fix-canvas-deps": "node ./scripts/fix-canvas-deps.mjs"


What is the requirement for canvas in the first place? This is complexity which shouldn't be included without very good reason.

Canvas is required for some of the tests IIRC, might not be necessary if we upgrade to xtermJs 5 which adds the DOM based renderer for testing

With this complexity, I would say that we stop testing xtermjs directly and only test the contract (our wrapper).

jansav · 2023-01-11T07:10:44Z

...es/cluster/kube-object-status-icon/extension-api/reactively-hide-kube-object-status.test.tsx

-
-    expect(actual).toHaveLength(0);
+  it("does not show the kube object status", async () => {
+    await waitFor(() => expect(rendered.baseElement.querySelectorAll(".KubeObjectStatusIcon")).toHaveLength(0));


This is slow unit test. In unit test we should always know if the item is there or not.

jansav · 2023-01-11T07:10:49Z

...es/cluster/kube-object-status-icon/extension-api/reactively-hide-kube-object-status.test.tsx

-    );
-
-    expect(actual).toHaveLength(1);
+    await waitFor(() => expect(rendered.baseElement.querySelectorAll(".KubeObjectStatusIcon")).toHaveLength(1));


This is slow unit test. In unit test we should always know if the item is there or not.

jansav · 2023-01-11T07:12:27Z

src/features/quitting-and-restarting-the-app/opening-application-window-using-tray.test.ts

@@ -136,7 +137,7 @@ describe("opening application window using tray", () => {
          });

          it("starts loading of content for the application window", () => {
-            expect(callForApplicationWindowHtmlMock).toHaveBeenCalledWith("https://lens.app:42");
+            expect(callForApplicationWindowHtmlMock).toHaveBeenCalledWith(matches((val) => val.startsWith("http://lens.app:")));


Side-effect with the port number :(

jansav · 2023-01-11T07:13:45Z

src/renderer/api/create-terminal-api.injectable.ts

-    return (query) => new TerminalApi(deps, query);
+    return (query) => {
+      const hostedClusterId = di.inject(hostedClusterIdInjectable);
+


This injection should be done in the instantiate time and not in the function.

Yes good point.

jansav · 2023-01-11T07:25:19Z

src/common/cluster/cluster.ts

@@ -20,12 +20,12 @@ import { disposer, isDefined, isRequestError, toJS } from "../utils";
 import type { Response } from "request";


"Update (mostly just) snapshots and a few other tests"

Please be more careful with commit names, this one does a lot more than it says. Almost skipped reviewing because it said "update snapshots"

jansav · 2023-01-11T07:25:54Z

src/features/helm-charts/listing-active-helm-repositories-in-preferences.test.ts

@@ -43,7 +43,7 @@ describe("listing active helm repositories in preferences", () => {

    builder.beforeApplicationStart((mainDi) => {
      mainDi.override(readYamlFileInjectable, () => readYamlFileMock);
-      mainDi.override(execFileInjectable, () => execFileMock);
+      mainDi.override(exrequestPublicHelmRepositoriesInjectable


jansav · 2023-01-11T07:26:00Z

src/features/preferences/navigation-to-kubernetes-preferences.test.ts

@@ -30,7 +30,7 @@ describe("preferences - navigation to kubernetes preferences", () => {
        mainDi.override(
          getActiveHelmRepositoriesInjectable,
          () => async () => ({ callWasSuccessful: true, response: [] }),
-        );
+        );requestPublicHelmRepositoriesInjectable;


src/features/window-location/main/for-top-bar-state-handler.injectable.ts

jansav · 2023-01-11T07:31:28Z

src/common/fetch/lens-fetch.injectable.ts

+/**
+ * This fetch is for requesting items from the Lens Proxy and thus does not cause side effects
+ */
+const lensFetchInjectable = getInjectable({


How about we override this in behavioural test with a fake that does whatever the "server" does. So basically maps the request to correct route in backend.

jansav · 2023-01-11T07:38:51Z

src/features/shell-sync/main/compute-unix-shell-environment.global-override-for-injectable.ts

-});
+export default getGlobalOverride(computeUnixShellEnvironmentInjectable, () => async () => ({
+  callWasSuccessful: true,
+  response: process.env,


This is introducing side effect to override. In global override the response should be always same.

jansav · 2023-01-11T07:40:00Z

package.json

@@ -78,7 +79,7 @@
    "build:theme-vars": "yarn run ts-node build/build_theme_vars.ts",
    "lint": "PROD=true yarn run eslint --ext js,ts,tsx --max-warnings=0 .",
    "lint:fix": "yarn run lint --fix",
-    "postinstall": "yarn run fix-canvas-deps",
+    "postinstall": "yarn run fix-canvas-deps && yarn run compile:node-fetch",


Both of these introduce level of complexity that makes me question why they are really needed.

jansav · 2023-01-11T07:48:38Z

src/common/app-paths/app-paths.test.ts

 import type { DiContainer } from "@ogre-tools/injectable";
 import appPathsInjectable from "./app-paths.injectable";

 describe("app-paths", () => {
  let builder: ApplicationBuilder;

-  beforeEach(() => {
-    builder = getApplicationBuilder();
+  setupInitializingApplicationBuilder(b => builder = b);


Ugh. This would be hiding a lot of important details. It also introduces dependency between order of these before each loops. If the motivation is to remove the requirement for afterEach from usages, then we should figure out why we need the afterEach at all. It shouldn't be there.

jansav · 2023-01-11T08:16:45Z

src/common/cluster/cluster.ts

+    // console.log({
+    //   resource,
+    //   allowed: toJS([...this.allowedResources]),
+    // });
+


jansav · 2023-01-11T08:17:43Z

src/common/utils/collection-functions.ts

+
+  const newVal = await getter();
+
+  runInAction(() => box.set(newVal));


Dangerous. Transaction should be controlled from the useplace.

Not when you expect the helper function to be an action

jansav · 2023-01-11T08:20:53Z

src/common/fs/change-path-mode.global-override-for-injectable.ts

@@ -1,11 +0,0 @@
-/**


This commit doesn't explain at all why these global overrides were removed?

jansav · 2023-01-11T08:22:19Z

src/jest.setup.ts

+/**
+ * This is needed for getting XTermJs to work in tests
+ */


How about we override x-term-js with something that mimics the behaviour but is in our full control?

I don't think it is worth it. But maybe

Iku-turso · 2023-01-11T10:34:48Z

scripts/fix-canvas-deps.ts

@@ -0,0 +1,46 @@
+/**


This whole file feels like an amazing kludge, and brittle too. Is there really no other way?

Unfortunately I could not find one after banging my head for a few days. The fact that we use Electron seem to break some of the dependencies auto detecting certain configuration things.

Nokel81 added the bug Something isn't working label Jun 14, 2022

Nokel81 added this to the 5.7.0 milestone Jun 14, 2022

Nokel81 requested a review from a team as a code owner June 14, 2022 16:21

Nokel81 requested review from jweak and JAtula and removed request for a team June 14, 2022 16:21

Nokel81 modified the milestones: 5.7.0, 5.6.0 Jun 27, 2022

Nokel81 modified the milestones: 6.0.0, 6.1.0 Jul 21, 2022

Nokel81 marked this pull request as draft July 22, 2022 14:46

Nokel81 modified the milestones: 6.1.0, 6.0.1 Jul 29, 2022

Nokel81 modified the milestones: 6.0.1, 6.0.2 Aug 9, 2022

Nokel81 removed this from the 6.0.2 milestone Sep 2, 2022

github-actions bot added the PR: needs rebase label Oct 5, 2022

Nokel81 force-pushed the fix-invalid-shell-setting branch from 03e449b to ecffed8 Compare October 5, 2022 20:46

Nokel81 marked this pull request as ready for review October 5, 2022 20:46

github-actions bot removed the PR: needs rebase label Oct 5, 2022

Nokel81 added PR: needs rebase area/terminal labels Oct 5, 2022

github-actions bot removed the PR: needs rebase label Oct 5, 2022

github-actions bot added the PR: needs rebase label Oct 13, 2022

Nokel81 force-pushed the fix-invalid-shell-setting branch from ecffed8 to 2a5871e Compare October 13, 2022 12:23

github-actions bot removed the PR: needs rebase label Oct 13, 2022

jansav reviewed Jan 11, 2023

View reviewed changes

src/features/window-location/main/for-top-bar-state-handler.injectable.ts Show resolved Hide resolved

jansav reviewed Jan 11, 2023

View reviewed changes

Iku-turso reviewed Jan 11, 2023

View reviewed changes

Nokel81 closed this Jan 11, 2023

Nokel81 modified the milestones: 6.3.1, 6.4.0 Jan 11, 2023

		import type { RequestFromChannel } from "../../../renderer/utils/channel/request-from-channel.injectable";
		import requestFromChannelInjectable from "../../../renderer/utils/channel/request-from-channel.injectable";

		@@ -20,12 +20,12 @@ import { disposer, isDefined, isRequestError, toJS } from "../utils";
		import type { Response } from "request";


		const newVal = await getter();

		runInAction(() => box.set(newVal));

Fix failing to start shell being hidden from user #5626

Fix failing to start shell being hidden from user #5626

Conversation

Nokel81 commented Jun 14, 2022 • edited Loading

github-actions bot commented Oct 5, 2022

github-actions bot commented Oct 5, 2022

github-actions bot commented Oct 5, 2022

github-actions bot commented Oct 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jansav Jan 11, 2023 • 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

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

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

jansav Jan 11, 2023 • 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

Nokel81 commented Jun 14, 2022 •

edited

Loading

jansav Jan 11, 2023 •

edited

Loading

jansav Jan 11, 2023 •

edited

Loading