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.

Sign up for GitHub

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

Jump to bottom

🛠 Tooling: End-to-end migration test lint takes a long time on coverage-migrate/ files #1131

Closed
3 tasks done
JoshuaKGoldberg opened this issue Dec 30, 2023 · 3 comments · Fixed by #1132
Closed
3 tasks done

🛠 Tooling: End-to-end migration test lint takes a long time on coverage-migrate/ files #1131

JoshuaKGoldberg opened this issue Dec 30, 2023 · 3 comments · Fixed by #1132
Assignees
@JoshuaKGoldberg
Labels
area: tooling Managing the repository's maintenance status: accepting prs Please, send a pull request to resolve this!

Comments

@JoshuaKGoldberg
Copy link
Owner

Bug Report Checklist

  • I have tried restarting my IDE and the issue persists.
  • I have pulled the latest main branch of the repository.
  • I have searched for related issues and found none that matched my issue.

Overview

I noticed while tinkering with #1048 that pnpm run test:migrate spends a lot of time locally in "Cleaning up files". That log wraps around two commands:

create-typescript-app/src/migrate/migrateWithOptions.ts

Lines 63 to 66 in c06c62c

await runCommands("Cleaning up files", [
"pnpm lint --fix",
"pnpm format --write",
]);

I tried running pnpm lint locally... and it reported quite a few issues:

✖ 457725 problems (457725 errors, 0 warnings)
  457696 errors and 0 warnings potentially fixable with the `--fix` option.

Wow. It features lovely lines like this:

/Users/josh/repos/create-typescript-app/coverage-migrate/tmp/coverage-48900-1703919745073-0.json
  1:74       error  Expected object keys to be in ascending order. 'functions' should be before 'url'                                                                                                                                                                                                                                                                                                                                                                                                        jsonc/sort-keys

...along with roughly 73,000 more errors.

This explains the slowness! Because the migration script runs pnpm lint --fix when it's done, and the migration script puts coverage reports in coverage-migrate... there's now a whole bunch of linting being done.

Additional Info

Splitting this performance improvement task out of #860.

I'm not sure exactly what the right fix is here...
@JoshuaKGoldberg JoshuaKGoldberg added area: tooling Managing the repository's maintenance status: accepting prs Please, send a pull request to resolve this! labels Dec 30, 2023
@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Dec 30, 2023
Merged
3 tasks
JoshuaKGoldberg added a commit that referenced this issue Dec 30, 2023
@JoshuaKGoldberg
chore: keep coverage directories in-place for test-initialize and tes…
df5941d 
…t-migrate (#1132)

## PR Checklist

- [x] Addresses an existing open issue: fixes #1131
- [x] That issue was marked as [`status: accepting
prs`](https://github.com/JoshuaKGoldberg/create-typescript-app/issues?q=is%3Aopen+is%3Aissue+label%3A%22status%3A+accepting+prs%22)
- [x] Steps in
[CONTRIBUTING.md](https://github.com/JoshuaKGoldberg/create-typescript-app/blob/main/.github/CONTRIBUTING.md)
were taken

## Overview

Keeps them running on `./coverage`, then swaps that to `./coverage-*`
later in the workflow.

Comparing the performance of the two end-to-end test types from `main`
(c06c62c):

| Test         | Baseline | Update   | Δ                      |
| ------------ | -------- | -------- | ---------------------- |
| `initialize` | `1m 25s` | `1m 21s` | ~0% (near zero change) |
| `migrate`    | `2m 45s` | `30s`    | 81% faster             |

Seems like this mostly improves migration. I guess initialization didn't
have this bottleneck as badlly, and still runs the initialize script
twice.
@github-actions GitHub Actions
Copy link

🎉 This is included in version v1.50.1 🎉

The release is available on:

Cheers! 📦🚀

@JoshuaKGoldberg
Copy link
Owner Author

Regression: this is still happening.

@JoshuaKGoldberg JoshuaKGoldberg self-assigned this Jan 9, 2024
@JoshuaKGoldberg
Copy link
Owner Author

Ehh I'll file a new issue. The modifying of .yml was good.

@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Jan 9, 2024
Closed
3 tasks
@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Jan 17, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@JoshuaKGoldberg JoshuaKGoldberg

Labels
area: tooling Managing the repository's maintenance status: accepting prs Please, send a pull request to resolve this!
Projects
None yet
Milestone
No milestone
1 participant
@JoshuaKGoldberg