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

Refactor the module structure for the new executors #1302

Closed
na-- opened this issue Jan 8, 2020 · 3 comments
Closed

Refactor the module structure for the new executors #1302

na-- opened this issue Jan 8, 2020 · 3 comments
Assignees

Comments

@na--
Copy link
Member

na-- commented Jan 8, 2020

This discussion is a nice starting point for what's currently suboptimal: #1007 (comment)

I also noticed that the lib package depends on the js/compiler package... go list -f '{{ join .Imports "\n" }}' github.com/loadimpact/k6/lib | sort produces:

...
github.com/loadimpact/k6/js/compiler
github.com/loadimpact/k6/lib/fsext
github.com/loadimpact/k6/lib/types
github.com/loadimpact/k6/loader
github.com/loadimpact/k6/stats
github.com/loadimpact/k6/ui/pb
...

it's not related to the new executors, but this shouldn't be the case

@na-- na-- added the refactor label Jan 8, 2020
@na-- na-- added the executors label Mar 8, 2020
@na--
Copy link
Member Author

na-- commented Mar 25, 2020

Some of the tests of the API (and probably other modules as well) are currently structured in such a way that they import the js package: https://github.com/loadimpact/k6/blob/298cf6bfab04d64392db34872240aca0f96c3e78/api/v1/setup_teardown_routes_test.go#L37

This would cause any library that also imports them to include things like goja and core.js, which is probably not what we want... 🤦‍♂️ We can instead use different package names for these tests, for example v1_test.

@mstoykov
Copy link
Contributor

hm ... I think that it probably won't require the js package as it is only in the test, but I could be wrong ...

@oleiade
Copy link
Member

oleiade commented Feb 2, 2024

We have recently discussed this issue in our backlog grooming session. We agreed that if the specific issue reported here: the lib package importing the js module, we should close.

I just verified on master and it seems to be the case indeed. Closing this. Feel free to reopen in the future if you believe this issue is still relevant regardless.

@oleiade oleiade closed this as completed Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants