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

Enable cargo-nextest for CI #860

Closed
wants to merge 5 commits into from
Closed

Enable cargo-nextest for CI #860

wants to merge 5 commits into from

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Aug 22, 2022

To avoid issues with the CI, the nextest was replaced with cargo test.

This PR ensures we utilize it again the cargo nextest functionality, via
installing it locked (commit)

Issue reference: nextest-rs/nextest#306

Time Delta

Location Sample 1 Sample 2 After 1 After 2
CI (ubuntu) 3m 10s 3m 1s 4m 47s 3m 23s
Windows 9m 33s 10m 15s 11m 18s 11m 34s
MacOS 8m 18s 5m 43s 8m 36s 9m 48s

Sample 1: 6fdb67f
Sample 2: bd31557.

Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv lexnv requested review from a team as code owners August 22, 2022 16:46
@lexnv lexnv self-assigned this Aug 22, 2022
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice,

it still doesn't support doc tests right?
might be good to add another job for that..

@alvicsam
Copy link
Contributor

alvicsam commented Aug 23, 2022

it still doesn't support doc tests right?

No, still not nextest-rs/nextest#16

I can suggest editing the test job in .gitlab-ci.yml. It runs on the parity CI runners which are way faster than GitHub runners. In this case the Continuous integration / Run tests Ubuntu (pull_request) GHA job can be removed. We also have a mac-runner but it's needed to test nextest on it. Unfortunately, we don't have windows runners so for them the only option is to use GHA.

.gitlab-ci.yml Outdated
@@ -103,7 +103,9 @@ test:
<<: *docker-env
<<: *common-refs
script:
- cargo test
- cargo install cargo-nextest --locked
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary, the image already contains cargo-nextest (not the latest version though)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll remove it, does cargo-hack also exist on the image?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we can add it if needed. But it might take some time when it reaches the production image

Signed-off-by: Alexandru Vasile <[email protected]>
@jsdw
Copy link
Collaborator

jsdw commented Aug 23, 2022

So to confirm, it looks like nextest is actually taking longer than the old cargo test as it stands?

(Does that include the additional cargo test invocation that we need to run separate doc tests, and the time taken to download/install nextest?)

That being the case, I'd prefer to keep it simple and stick with cargo test until there is a clear advantage, or am I missing something?

@niklasad1
Copy link
Member

So to confirm, it looks like nextest is actually taking longer than the old cargo test as it stands?

wow, I thought nextest was suppose to faster... but I think the UI/proc macro tests are the bottleneck

@jsdw
Copy link
Collaborator

jsdw commented Sep 16, 2022

Is the verdict to close this PR since nexttest is actually slower anyway (plus doesn't run doctests)?

@jsdw jsdw closed this Sep 29, 2022
@niklasad1 niklasad1 deleted the enable_nextest branch January 4, 2023 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants