Remove WriteFile and move it into CopyFile #204
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- I've removed the WriteFile and put it into CopyFile. - It still uses filesytem.Split but just to get the directory name to create it if it doesn't exist. - It opens the file with os.OpenFile and set the file permissions there instead of after copying. - It removes the file if io.Copy returns an error. - It doesn't need error handling on toFile.Close as it only returns an error if it's called multiple times.
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/vercel/turbo-site/3R4mnhZPU4TScGGwjYhfqinmMHbb |
|
This is roughly 2-3x slower on my benchmarks |
I'm not sure how it could be that much slower, it's practically the same apart from:
It could be that this one removes the file if there's an error copying, wheras the old implementation didn't bother. If you're on windows the old one could be running quicker due to the bug this is meant to fix. This one also avoids the need for creating a temporary file and renaming it. |
|
Run I will formally benchmark again tomorrow |
|
If I don't figure out what caused it it would probably be best to revert the commit and just replace |
The only change is changing path.Split -> filepath.Split
|
I’ve reverted merging |
sokra
pushed a commit
that referenced
this pull request
Oct 25, 2022
This is a very early version of the next-dev test runner. I'm opening this early to get thoughts from folks re: the direction of the design and implementation. Fixes #204 Currently it: * Discovers integration test fixtures from the filesystem. Right now these are expected to be single files that get bundled and will eventually include assertions. This is powered by the test-generator crate, which allows us not to have to manually enumerate each case. We could consider using this for the node-file-trace tests as well. * Starts the dev server on a free port and opens a headless browser to its root. The browser control is implemented with the https://crates.io/crates/chromiumoxide crate, which expects Chrome or Chromium to already be available. Eventually it will: * [x] Implement a minimal test environment loaded in the browser so that assertions can be run there from bundled code. * [x] Report back the results of these assertions to rust, where we can pass/fail cargo tests with those results. In the future it could: * Possibly include snapshot-style tests to assert on transformed results. This could be in the form of fixture directories instead of files cc @jridgewell * Support expressing special configuration of turbopack in a fixture, possibly as another file in the fixture directory. * [x] ~Possibly support distributing tests to a pool of open browsers instead of opening and closing for each test.~ Test Plan: See next PRs
sokra
pushed a commit
that referenced
this pull request
Oct 25, 2022
Co-authored-by: Henry Heffernan <henryheffernan@gmail.com>
jridgewell
pushed a commit
to vercel/next.js
that referenced
this pull request
Mar 10, 2023
This is a very early version of the next-dev test runner. I'm opening this early to get thoughts from folks re: the direction of the design and implementation. Fixes vercel/turborepo#204 Currently it: * Discovers integration test fixtures from the filesystem. Right now these are expected to be single files that get bundled and will eventually include assertions. This is powered by the test-generator crate, which allows us not to have to manually enumerate each case. We could consider using this for the node-file-trace tests as well. * Starts the dev server on a free port and opens a headless browser to its root. The browser control is implemented with the https://crates.io/crates/chromiumoxide crate, which expects Chrome or Chromium to already be available. Eventually it will: * [x] Implement a minimal test environment loaded in the browser so that assertions can be run there from bundled code. * [x] Report back the results of these assertions to rust, where we can pass/fail cargo tests with those results. In the future it could: * Possibly include snapshot-style tests to assert on transformed results. This could be in the form of fixture directories instead of files cc @jridgewell * Support expressing special configuration of turbopack in a fixture, possibly as another file in the fixture directory. * [x] ~Possibly support distributing tests to a pool of open browsers instead of opening and closing for each test.~ Test Plan: See next PRs
sokra
pushed a commit
to vercel/next.js
that referenced
this pull request
Mar 13, 2023
This is a very early version of the next-dev test runner. I'm opening this early to get thoughts from folks re: the direction of the design and implementation. Fixes vercel/turborepo#204 Currently it: * Discovers integration test fixtures from the filesystem. Right now these are expected to be single files that get bundled and will eventually include assertions. This is powered by the test-generator crate, which allows us not to have to manually enumerate each case. We could consider using this for the node-file-trace tests as well. * Starts the dev server on a free port and opens a headless browser to its root. The browser control is implemented with the https://crates.io/crates/chromiumoxide crate, which expects Chrome or Chromium to already be available. Eventually it will: * [x] Implement a minimal test environment loaded in the browser so that assertions can be run there from bundled code. * [x] Report back the results of these assertions to rust, where we can pass/fail cargo tests with those results. In the future it could: * Possibly include snapshot-style tests to assert on transformed results. This could be in the form of fixture directories instead of files cc @jridgewell * Support expressing special configuration of turbopack in a fixture, possibly as another file in the fixture directory. * [x] ~Possibly support distributing tests to a pool of open browsers instead of opening and closing for each test.~ Test Plan: See next PRs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Should fix #195
It still uses
filesytem.Splitbut just to get the directory name to create it if it doesn't exist.It opens the file with
os.OpenFileand set the file permissions there instead of after copying.It removes the file if
io.Copyreturns an error.It doesn't need error handling on
toFile.Closeas it only returns an error if it's called multiple times.