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

chore: use unzipped config fixtures #739

Merged
merged 11 commits into from
Aug 13, 2024
Merged

Conversation

tomasciccola
Copy link
Contributor

@tomasciccola tomasciccola commented Jul 31, 2024

For ease of debugging I created a script that zips every folder in tests/fixtures/config, so the idea is to commit uncompressed folders instead of zips.
I'm wondering if we should have some exceptions for this. e.g. tooBigOfAZip -> we can keep the zipped file since its smth we're not going to touch further. It makes the script less straight away thought, but wondering how github will deal with a folder with so many entries (I don't want to make the repo slow to navigate...)

Still haven't added those folders (want to double check before).
Also I used yazl instead of the zip cli since I don't want to worry about platform compatibility (despite using a bash script would be muuuch simpler)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@tomasciccola tomasciccola requested a review from EvanHahn July 31, 2024 15:22
scripts/build-config-fixtures.js Show resolved Hide resolved
scripts/build-config-fixtures.js Outdated Show resolved Hide resolved
scripts/build-config-fixtures.js Outdated Show resolved Hide resolved
@@ -8,3 +8,4 @@ proto/build
!/drizzle/**/*.sql
.eslintcache
docs/api/html/*
test/fixtures/config/*.zip
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens to all the existing fixtures when we do this?

Copy link
Contributor Author

@tomasciccola tomasciccola Jul 31, 2024

Choose a reason for hiding this comment

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

Before merging I'll git rm all the zips and git add all the folders, so with this they should be untracked, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. Could you do that before we merge?

scripts/build-config-fixtures.js Outdated Show resolved Hide resolved
'../tests/fixtures/config',
import.meta.url
).pathname
const dir = await fs.readdir(CONFIG_FIXTURES_PATH)
Copy link
Contributor

Choose a reason for hiding this comment

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

You could instead pass the withFileTypes option, which will give back an array of fs.Dirent objects instead of strings. Then you don't need to call fs.stat below, because you can check if it's a directory right there.

Take it or leave it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

tomasciccola and others added 4 commits July 31, 2024 14:21
* import `fs` as module, not using the default import
* use path.join instead of string concatenation
* better signature for `zipFolder`
* remove unnecessary await

Co-authored-by: Evan Hahn <[email protected]>
@tomasciccola tomasciccola requested a review from EvanHahn July 31, 2024 17:46
@EvanHahn
Copy link
Contributor

I probably won't be able to review this until I get back on August 12. Just FYI!

@tomasciccola tomasciccola self-assigned this Aug 1, 2024
Copy link
Contributor

@EvanHahn EvanHahn left a comment

Choose a reason for hiding this comment

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

LGTM but we should unzip all the ZIPs before merging.

@@ -8,3 +8,4 @@ proto/build
!/drizzle/**/*.sql
.eslintcache
docs/api/html/*
test/fixtures/config/*.zip
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. Could you do that before we merge?

@tomasciccola tomasciccola merged commit 1c097ee into main Aug 13, 2024
7 checks passed
@tomasciccola tomasciccola deleted the chore/useUnzippedConfigFixture branch August 13, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants