Rename gRPC module to k6/net/grpc and track and bundle loaded protobuf files #1706
Conversation
|
Ugh, yeah, Windows file paths... 😭 I'll try to fix the tests tomorrow, as well as hopefully write a few new ones. |
js/modules/k6/grpc/client_test.go
Outdated
| FileSystems: map[string]afero.Fs{ | ||
| "file": afero.NewOsFs(), |
There was a problem hiding this comment.
There was a problem hiding this comment.
LGTM apart from the failing windows tests.
I haven't tested it ... and likely won't try as I don't have any gRPC code or tests to begin with, so it will be just the most basic testing that is likely not worth the trouble.
I do like the idea of initEnv and am glad that this didn't require some complete refactor across the code base, and this does look like it won't break anything 🤞
| @@ -527,6 +527,7 @@ func TestVURunContext(t *testing.T) { | |||
| fnCalled = true | |||
|
|
|||
| assert.Equal(t, vu.Runtime, common.GetRuntime(*vu.Context), "incorrect runtime in context") | |||
| assert.Nil(t, common.GetInitEnv(*vu.Context)) // shouldn't get this in the vu context | |||
There was a problem hiding this comment.
It will be nice for there to be a test it is there in the initcontext though, in the js packages, for example, bundle_test.go
There was a problem hiding this comment.
Yeah, that's one test I want to add - I couldn't do it here since getSimpleRunner() can't be used for it
Codecov Report
@@ Coverage Diff @@
## master #1706 +/- ##
==========================================
+ Coverage 71.40% 71.42% +0.02%
==========================================
Files 177 177
Lines 13696 13702 +6
==========================================
+ Hits 9779 9787 +8
+ Misses 3305 3303 -2
Partials 612 612
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
I am fine with this as it is ... more testing with complexer scenarios 😉 😉 |
|
Almost forgot, we reconsidered the decision to use |
na--
changed the title
| Accessor: protoparse.FileAccessor(func(filename string) (io.ReadCloser, error) { | ||
| absFilePath := initEnv.GetAbsFilePath(filename) | ||
| // initEnv.Logger.Infof("gRPC trying to load %s from %s...", filename, absFilePath) | ||
| return initEnv.FileSystems["file"].Open(absFilePath) |
There was a problem hiding this comment.
Maybe for a follow up PR, but could we use constants / an enum for the FileSystems keys? It wasn't clear to me initially that this is the URL scheme.
There was a problem hiding this comment.
Yeah, we probably should refactor this together with the other TODOs I left...
na--
force-pushed
the
grpc-file-fixes
branch
2 times, most recently
from
b7db89c
to
9a762b5
Compare
|
I plan to merge this in |
na--
changed the title
This should fix #1696 (it's WIP since I want to write a few more tests).
I think I've managed to get away with very little refactoring for v0.29.0, so hopefully not introducing any bugs... 🤞
I'm also hopeful that the new
InitEnvironmentconcept is a viable way to refactor theInitContextandBaseInitContextand get rid of some of the complexity and code smells (ctxPtr🤮) there in future k6 releases. In principle, we should be able to extend theInitEnvironmentstruct so that there is nothing special about the built-in k6 functions likeopen()and evenrequire(), i.e. refactor them so they useGetInitEnv()as well.@rogchap, can you please also review this PR and test it, to see if it works for your complex use case as well. I imagine that removing
protoparse.ResolveFilenames()and always disablingInferImportPathsmight break some complicated imports, even after automatically adding the CWD as the default import path, though I couldn't do it on my machine with my simple.protofiles... In any case,protoparse.ResolveFilenames()is not viable since it directly uses theospackage to look for files, so if it's absolutely needed, we'd have to re-implement it with afero (or a generic file system interface) in a future k6 version.Edit: for testing it properly, if
k6 run script.jsworks, thenk6 archive script.js && k6 run archive.tarshould also work.