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

Improve start-frontend CLI test #1257

Merged
merged 9 commits into from
Jun 25, 2024
Merged

Conversation

jinmayamashita
Copy link
Collaborator

@jinmayamashita jinmayamashita commented May 17, 2024

Pull Request Checklist

  • I have checked that this pull request is not a duplicate of a pre-existing pull request
  • I have self-reviewed my changes
    • There are no spelling mistakes
    • There are no remaining debug log prints (i.e. console.log())
    • Comments were written for complex code
  • I have checked that all tests are passing (for bug fixes and enhancements)
    • CLI Test (npm run test:cli)
    • Unit Test (npm run test:modules)
    • E2E Test (npm run e2e)
  • I have added and/or modified relevant tests for my changes (for bug fixes and enhancements)
  • I have added and/or modified relevant documentations for my changes (if necessary)

Description

In the existing tests, there was no check to verify if the output generated by running the CLI was correct, so I added it.

Context

Resolves #1255

Before

describe.skip("install react boilerplate to specify dir", () => {
test("install", async () => {
await exe("node", [startFrontend, testDir], { cwd });
const expectDirs = [
".env.template",
".eslintrc.js",
".gitignore",
"README.md",
"__mocks__",
"babel.config.js",
"index.html",
"jest-setup.ts",
"package.json",
"public",
"src",
"tests",
"tsconfig.json",
"vite.config.ts",
"yarn.lock",
];
expect(fse.readdirSync(path.resolve(cwd, testDir))).toStrictEqual(
expectDirs
);
});
});

After

expect(result).toContain(`
├── .eslintignore
├── .eslintrc.cjs
├── .gitignore
├── .prettierignore
├── .prettierrc
├── .storybook
| ├── main.ts
| ├── preview-head.html
| └── preview.ts
├── __mocks__
| ├── browser.ts
| ├── index.ts
| ├── request-handlers.ts
| └── server.ts
├── __tests__
| ├── About.test.ts
| ├── Home.test.ts
| └── utils
| └── global-setup.ts
├── env.d.ts
├── index.html
├── package.json
├── playwright.config.ts
├── public
| ├── favicon.svg
| └── mockServiceWorker.js
├── src
| ├── app.tsx
| ├── assets
| | ├── base.css
| | └── main.css
| ├── components
| | ├── Button.tsx
| | └── button.module.css
| ├── context.tsx
| ├── main.tsx
| ├── modules
| | └── restful
| | ├── components
| | | ├── user-form.test.tsx
| | | ├── user-form.tsx
| | | ├── user-list.test.tsx
| | | ├── user-list.tsx
| | | ├── user-view.test.tsx
| | | └── user-view.tsx
| | ├── hooks
| | | ├── use-user.test.tsx
| | | └── use-user.ts
| | └── index.ts
| ├── routes.tsx
| └── ui
| ├── nav-link.tsx
| └── pages
| ├── about
| | └── index.tsx
| ├── index.tsx
| ├── layout.tsx
| └── not-found
| └── index.tsx
├── stories
| └── Button.stories.tsx
├── tsconfig.json
├── tsconfig.node.json
├── vite.config.ts
├── vitest.config.ts
└── vitest.setup.ts`);

Copy link

changeset-bot bot commented May 17, 2024

🦋 Changeset detected

Latest commit: 3b0ba4f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
start-frontend Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jinmayamashita jinmayamashita self-assigned this May 17, 2024
@jinmayamashita jinmayamashita changed the title Improve cli test Improve start-frontend CLI test May 17, 2024
@jinmayamashita jinmayamashita marked this pull request as ready for review June 2, 2024 13:14
@jinmayamashita jinmayamashita enabled auto-merge June 2, 2024 13:14
@jinmayamashita jinmayamashita requested review from ptrkdan and G-N555 June 3, 2024 00:49
@jinmayamashita jinmayamashita disabled auto-merge June 3, 2024 00:50
Copy link
Collaborator Author

@jinmayamashita jinmayamashita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ptrkdan @G-N555

Please review this PR.
Once it's merged, I will merge it into the PRs where CI tests are currently failing and check them again.

Comment on lines -53 to -67
expect(results).toContain(`start-frontend`);
expect(results).toContain(`Welcome!`);
expect(results).toContain(
`? Where Would You like to Create Your Application?`
);
expect(results).toContain(`? Select a JavsScript library for UI`);
expect(results).toContain(`? Select an API Solution`);
expect(results).toContain(`? Select module do you want to use`);
expect(results).toContain(`? Add Testing codes for Catching bugs early?`);
expect(results).toContain(`? Add Vitest for Unit Testing?`);
expect(results).toContain(`? Add Storybook for Visual Testing?`);
expect(results).toContain(`? Add Playwright for End-To-End Testing?`);
expect(results).toContain(`? Add ESLint for Code Linting?`);
expect(results).toContain(`? Add Prettier for Code Formatting?`);
expect(results).toContain(`Success! Created a new app at "my-test".`);
Copy link
Collaborator Author

@jinmayamashita jinmayamashita Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the wording of the CLI prompts is likely to change frequnetly, I thought it would be better to test the contests of the files generated by running the CLI rather than the working itself.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the file contents may change a bit after this, since we're reorganizing the templates and how the projects are generated 😅

We'll just have to keep this in mind and modify the tests in order to accommodate any changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right. Even if we change the tests to check the file structure from the messages, the file structure might change frequently for a while, so we'll need to update the tests each time. 😶‍🌫️

In the future, it might be more flexible to focus on whether the generated files can be built, rather than their structure. 🤔

const exe = util.promisify(child.execFile);
const TEST_DIR =
// eslint-disable-next-line turbo/no-undeclared-env-vars
process.env.RUNNER_TEMP || execSync("mktemp -d -t my-test").toString("utf-8");
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test folder is set to be created within a suitable folder in the OS's temp directory. Since mktemp does not work well with Github Actions, I configured it to use the temp folder already specified in the environment variables.

Comment on lines 116 to 50
expect(stdout.trim()).toBe(process.env.npm_package_version);
const { stdout } = await exe("node", [START_FRONTEND, "--version"]);
expect(stdout.trim()).toMatch(/^(\d+\.)?(\d+\.)?(\*|\d+)$/);
Copy link
Collaborator Author

@jinmayamashita jinmayamashita Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't need to be very strict; using a semantic versioning is sufficient.

Comment on lines 104 to 106
const result = execSync(
`npx tree-cli -a -l 5 --base ${TEST_DIR}`
).toString("utf-8");
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used tree-cli because it provides a visually clear folder structure. Do you have any other recommendations for a shell tree tool that works on macOS, Linux, and (if possible) Windows? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only ever used tree-cli myself. 🤔

Copy link
Collaborator

@ptrkdan ptrkdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working the tests! I think it's a good idea to test out the generated project; it definitely looks more meaningful!

I have a couple of questions regarding your implementation, though, so could you please take a look?

Comment on lines -53 to -67
expect(results).toContain(`start-frontend`);
expect(results).toContain(`Welcome!`);
expect(results).toContain(
`? Where Would You like to Create Your Application?`
);
expect(results).toContain(`? Select a JavsScript library for UI`);
expect(results).toContain(`? Select an API Solution`);
expect(results).toContain(`? Select module do you want to use`);
expect(results).toContain(`? Add Testing codes for Catching bugs early?`);
expect(results).toContain(`? Add Vitest for Unit Testing?`);
expect(results).toContain(`? Add Storybook for Visual Testing?`);
expect(results).toContain(`? Add Playwright for End-To-End Testing?`);
expect(results).toContain(`? Add ESLint for Code Linting?`);
expect(results).toContain(`? Add Prettier for Code Formatting?`);
expect(results).toContain(`Success! Created a new app at "my-test".`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the file contents may change a bit after this, since we're reorganizing the templates and how the projects are generated 😅

We'll just have to keep this in mind and modify the tests in order to accommodate any changes.

Comment on lines 104 to 106
const result = execSync(
`npx tree-cli -a -l 5 --base ${TEST_DIR}`
).toString("utf-8");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only ever used tree-cli myself. 🤔


const testDir = "my-test" as const;
let TEST_DIR;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not a constant, I think testDir would be more appropriate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 2661d0a

import concat from "concat-stream";
import { spawn } from "node:child_process";
import { execFile, exec, execSync } from "node:child_process";
import fse from "fs-extra";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestion, but what do you think about sorting the imports? (Opt + Shift + O on Mac VS Code)

It would be nice if there's a way to include it in the pre-commit script.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opt + Shift + O on Mac VS Code

Personally, I've been using Helix as my editor recently 🙏🏼
But for import order, I think your suggestion to handle it with a precommit hook could work. 👍🏼

It would be nice if there's a way to include it in the pre-commit script.

Yeah, it would be nice if we could ensure this with a formatter, like

FYI @mvn-luanvu-hn @seiya0914

@jinmayamashita jinmayamashita marked this pull request as draft June 7, 2024 01:03
@jinmayamashita jinmayamashita marked this pull request as ready for review June 7, 2024 01:29
@jinmayamashita jinmayamashita requested a review from ptrkdan June 7, 2024 01:29
@jinmayamashita jinmayamashita enabled auto-merge June 10, 2024 08:40
@ptrkdan ptrkdan mentioned this pull request Jun 14, 2024
7 tasks
Copy link
Collaborator

@ptrkdan ptrkdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jinmayamashita jinmayamashita merged commit d128cde into main Jun 25, 2024
1 check passed
@jinmayamashita jinmayamashita deleted the refactor/improve-cli-test branch June 25, 2024 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TEST] Some CLI Tests Are Skipped
2 participants