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.

Sign up for GitHub

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

Jump to bottom

Merge tests/ and src/tests to improve build times #7147

Closed
rgwood opened this issue Nov 16, 2022 · 8 comments · Fixed by #12826
Closed

Merge tests/ and src/tests to improve build times #7147

rgwood opened this issue Nov 16, 2022 · 8 comments · Fixed by #12826
Labels
build-package Everything concerning the CI build process and packaging of nushell good first issue Good for newcomers tests issues to add tests or fix tests
Milestone
v0.94.0

Comments

@rgwood
Copy link
Contributor

rgwood commented Nov 16, 2022

The Problem

cargo test is extremely slow to build, especially when dataframes are enabled. It builds multiple executables+DLLs and links dependencies into each one:
image

Proposed Solution

We should find ways to cut down on the number of distinct binaries built when running tests.

One place to start: merge the multiple nu-* binaries into 1. I believe that the 2 separate deps/nu-* files in the screenshot above correspond to the tests in tests/ and src/tests; we should be able to refactor those tests so they live in the same directory and get compiled into 1 binary instead of 2.

Related reading

https://endler.dev/2020/rust-compile-times/#combine-all-integration-tests-in-a-single-binary
@kubouch
Copy link
Contributor

kubouch commented Nov 16, 2022

If someone picks it up, don't forget to edit CONTRIBUTING.md afterwards since it includes the current setup of test directories.

@sholderbach sholderbach added completions Issues related to tab completion tests issues to add tests or fix tests build-package Everything concerning the CI build process and packaging of nushell and removed completions Issues related to tab completion labels Nov 16, 2022
@rgwood rgwood added the good first issue Good for newcomers label Dec 1, 2022
@rgwood
Copy link
Contributor Author

rgwood commented Jan 2, 2023

I tried doing this by moving the src/tests tests into tests/src_tests, it successfully elimintated 1 large nu-* executable from target/debug/deps but the total size of target/debug actually went up (8.8 GB -> 9.6 GB). Not sure why but that made it less of a clear win.

@ramirez7358
Copy link

Hello @rgwood , how are you?
I would like work on this.
Could you explain to me why there is such a difference in the size of your binaries with mine?

Screenshot from 2023-07-15 17-33-24

@rgwood
Copy link
Contributor Author

rgwood commented Jul 15, 2023 •
edited

Hi, that would be great if you want to take a look at this thanks.

I was building that on Linux, if you’re on macOS that might explain the difference.

Also this was a while ago and Nushell has changed since. I believe dataframes were enabled by default at the time and they are no longer.

@francesco-gaglione
Copy link
Contributor

@rgwood is this still needed?

@devyn
Copy link
Contributor

devyn commented May 10, 2024

It would still be nice to not have to build two binaries, even if the binaries are a lot smaller these days.

@francesco-gaglione
Copy link
Contributor

@devyn great, thank you, first time contributing here. I'm trying to do it. I will open a draft pull request as soon as I moved all the tests.

@francesco-gaglione francesco-gaglione mentioned this issue May 10, 2024
Merged
@francesco-gaglione
Copy link
Contributor

@devyn please check the PR, if it is ok or require other changes

IanManske added a commit that referenced this issue May 13, 2024
@francesco-gaglione @IanManske
Merged tests to produce a single binary (#12826)
c4dca5f 
This PR should close #7147 

# Description
Merged src/tests into /tests to produce a single binary.

![image](https://github.com/nushell/nushell/assets/94604837/84726469-d447-4619-b6d1-2d1415d0f42e)

# User-Facing Changes
No user facing changes

# Tests + Formatting
Moved tests. Tollkit check pr pass.

# After Submitting

---------

Co-authored-by: Ian Manske <[email protected]>
@hustcer hustcer added this to the v0.94.0 milestone May 13, 2024
FilipAndersson245 pushed a commit to FilipAndersson245/nushell that referenced this issue May 18, 2024
@francesco-gaglione @IanManske @FilipAndersson245
Merged tests to produce a single binary (nushell#12826)
7634f07 
This PR should close nushell#7147 

# Description
Merged src/tests into /tests to produce a single binary.

![image](https://github.com/nushell/nushell/assets/94604837/84726469-d447-4619-b6d1-2d1415d0f42e)

# User-Facing Changes
No user facing changes

# Tests + Formatting
Moved tests. Tollkit check pr pass.

# After Submitting

---------

Co-authored-by: Ian Manske <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
build-package Everything concerning the CI build process and packaging of nushell good first issue Good for newcomers tests issues to add tests or fix tests
Projects
None yet
Milestone
v0.94.0
Development

Successfully merging a pull request may close this issue.

Merged tests to produce a single binary francesco-gaglione/nushell
7 participants
@devyn @hustcer @ramirez7358 @sholderbach @kubouch @rgwood @francesco-gaglione