Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failing "Create-toolpad-app:test" test on cloning repository #2414

Closed
2 tasks done
Kirera-Wainaina opened this issue Aug 3, 2023 · 5 comments · Fixed by #2419
Closed
2 tasks done

Failing "Create-toolpad-app:test" test on cloning repository #2414

Kirera-Wainaina opened this issue Aug 3, 2023 · 5 comments · Fixed by #2419
Labels
scope: toolpad-studio Abbreviated to "studio"

Comments

@Kirera-Wainaina
Copy link
Contributor

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

Steps:

  1. Clone the repository onto my machine
  2. cd mui-toolpad
  3. Install dependencies - yarn install
  4. yarn test -> Initially fails 2 / 3 tests
  5. yarn dev
  6. yarn test -> fails 1 / 3 tests

Current behavior 😯

Running yarn test Immediately after cloning the repository, results in two of the tests failing.

Running yarn dev and yarn test after that results in one test failing: create-toolpad-app:test
failing-test

Expected behavior 🤔

I expect the tests to pass considering I haven't made any changes

Context 🔦

No response

Your environment 🌎

OS: "Ubuntu 22.04.2 LTS"
Node: v16.20.1
MUI: v0.1.21

@Kirera-Wainaina Kirera-Wainaina added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Aug 3, 2023
@Janpot
Copy link
Member

Janpot commented Aug 3, 2023

👍 Yes, this is a bit of a flaky and slow test. It's installing the node modules for the created toolpad project. @bharatkashyap Perhaps we should omit installation during the tests?

@Kirera-Wainaina
Copy link
Contributor Author

You are right about the installations taking time. I've adjusted time to jest.setTimeout(600000); which 10x the timeout time but it leads to another problem.
Screenshot from 2023-08-03 14-44-37

@Janpot
Copy link
Member

Janpot commented Aug 3, 2023

It's annoying that we can't see the output of running the create-toolpad-app command. Could you apply

diff --git a/packages/create-toolpad-app/tests/index.spec.ts b/packages/create-toolpad-app/tests/index.spec.ts
index 85e19a6d5..4976781f0 100644
--- a/packages/create-toolpad-app/tests/index.spec.ts
+++ b/packages/create-toolpad-app/tests/index.spec.ts
@@ -3,6 +3,7 @@ import * as path from 'path';
 import * as url from 'url';
 import readline from 'readline';
 import { Readable } from 'stream';
+import { text } from 'stream/consumers';
 import { execa, ExecaChildProcess } from 'execa';
 import { jest } from '@jest/globals';
 import { once } from 'events';
@@ -38,9 +39,11 @@ test('create-toolpad-app can bootstrap a Toolpad app', async () => {
   testDir = await fs.mkdtemp(path.resolve(currentDirectory, './test-app-'));
   cp = execa(cliPath, [path.basename(testDir)], {
     cwd: currentDirectory,
+    stdio: 'pipe',
   });
-  const result = await cp;
-  expect(result.stdout).toMatch('Run the following to get started');
+  cp.stdout!.pipe(process.stdout);
+  const stdout = await text(cp.stdout!);
+  expect(stdout).toMatch('Run the following to get started');
   const packageJsonContent = await fs.readFile(path.resolve(testDir, './package.json'), {
     encoding: 'utf-8',
   });

and try again? This should make visible where it gets stuck.

@Janpot Janpot added status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Aug 3, 2023
@Kirera-Wainaina
Copy link
Contributor Author

I figured fetch is from node-fetch so I've imported it and increased the time to timeout in Jest. It now works. What I've done to solve to issue.

diff --git a/packages/create-toolpad-app/tests/index.spec.ts b/packages/create-toolpad-app/tests/index.spec.ts
index 85e19a6d..0d2ca923 100644
--- a/packages/create-toolpad-app/tests/index.spec.ts
+++ b/packages/create-toolpad-app/tests/index.spec.ts
@@ -6,8 +6,9 @@ import { Readable } from 'stream';
 import { execa, ExecaChildProcess } from 'execa';
 import { jest } from '@jest/globals';
 import { once } from 'events';
+import fetch from 'node-fetch';
 
-jest.setTimeout(60000);
+jest.setTimeout(600000);
 
 const currentDirectory = url.fileURLToPath(new URL('.', import.meta.url));

Let me know if you still need me to go ahead and do what you are suggesting.

@github-actions github-actions bot removed the status: waiting for author Issue with insufficient information label Aug 3, 2023
@Janpot
Copy link
Member

Janpot commented Aug 3, 2023

Let me know if you still need me to go ahead

👍 Much appreciated if you'd open a PR. Importing from node-fetch would be helpful, but then also add it to this package's devDependencies, and make sure to set the version to exactly 2.6.12 (we're staying on the 2.x line for another package and will upgrade in a separate effort).

To avoid multiplying our ci runtime by 10 for failing create-toolpad-app tests we could do something like:

jest.setTimeout(process.env.CI ? 60000 : 600000);

but if the shorter timeout works for you with the node-fetch fix, I'd probably just keep it the way it is.

... and do what you are suggesting.

Yes, let's include these changes, it's very helpful to see this output when debugging the test.

Kirera-Wainaina added a commit to Kirera-Wainaina/mui-toolpad that referenced this issue Aug 3, 2023
…2414)

Import node-fetch it to test file

reinstall node-fetch as a dev dependency

Adjust timeout and import node-fetch in create-toolpad-app test (mui#2414)
@oliviertassinari oliviertassinari added the scope: toolpad-studio Abbreviated to "studio" label Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: toolpad-studio Abbreviated to "studio"
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants