-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix auto extension resolution breaking tests on windows #5206
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
Conversation
This reverts commit 5ab417b.
This reverts commit 6d7432c.
| dopts.Script.Name = sourceRootPath | ||
| dopts.Script.Contents = src.Data | ||
| dopts.Fs = newIOFSBridge(gs.FS, pwd) | ||
| dopts.Fs = newIOFSBridge(gs.FS, filepath.ToSlash(pwd)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason for not doing as same as readsource is doing?
k6/internal/loader/readsource.go
Line 20 in 8d016ab
| pwdURL := &url.URL{Scheme: "file", Path: filepath.ToSlash(filepath.Clean(pwd)) + "/"} |
|
Great @mstoykov, because it seems to fix the issue 👉🏻 #5176 (comment) 🎉 Now, I guess it's about removing prints and all that before final reviews? 🤔 Thanks! 🙌🏻 |
|
Unfortunately the things that fixes all the cases is the thing that is based on #5228 so the ongoing work to nto actually use esbuild and k6desp for the auto extension resolution. Hopefully all of that will enter 1.4.0. If not I will back off here to fix the more common case. |
|
I will move this to draft so people do not come to review it 😅 |
|
This is superseded by #5320 which fixes all cases |
What?
On Windows due to a mistake in #5016 on windows loading files would not work during the auto extension resolution in some cases and that breaks running the test as a whole.
Why?
This should be working.
Checklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
Fixes #5176