Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix test-linux-stable excessive run time #13597

Merged
merged 2 commits into from
Mar 17, 2023

Conversation

mateo-moon
Copy link
Contributor

@mateo-moon mateo-moon commented Mar 14, 2023

Add nextest config file. Aside of default config add mutual exclusion group(bottom of the file). This group is limited to one thread, meaning that tests from the group will be executed sequentially. This is because tests falling in this group run cargo build and multiple invocation of cargo build lead to clottering execution.
By using test group we could achieve 1 minute faster result than just using 1 thread for all tests)

@mateo-moon mateo-moon requested a review from a team as a code owner March 14, 2023 12:02
@mateo-moon mateo-moon self-assigned this Mar 14, 2023
@mateo-moon mateo-moon force-pushed the oleg/ci-cd/linux-test-stable#754 branch 11 times, most recently from b0b9a06 to de505eb Compare March 15, 2023 18:34
@mateo-moon
Copy link
Contributor Author

mateo-moon commented Mar 15, 2023

Closes paritytech/ci_cd#754

@mateo-moon mateo-moon changed the title [WIP] test-linux-stable excessive run time fix Fix test-linux-stable excessive run time Mar 15, 2023
@mateo-moon mateo-moon added the B0-silent Changes should not be mentioned in any release notes label Mar 15, 2023
@ggwpez
Copy link
Member

ggwpez commented Mar 15, 2023

This is because tests falling in this group run cargo build and multiple invocation of cargo build lead to clottering execution.

So is it as fast as before the bug occurred? Can you give a short before/after timing please?

@mateo-moon mateo-moon force-pushed the oleg/ci-cd/linux-test-stable#754 branch from de505eb to 1df5865 Compare March 15, 2023 19:32
@rcny rcny added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. labels Mar 15, 2023
@mateo-moon mateo-moon force-pushed the oleg/ci-cd/linux-test-stable#754 branch from 1df5865 to 416d906 Compare March 15, 2023 19:36
@bkchr
Copy link
Member

bkchr commented Mar 16, 2023

cargo build and multiple invocation of cargo build lead to clottering execution.

You mean too many parallel execution brought down the CPU?

@mateo-moon mateo-moon force-pushed the oleg/ci-cd/linux-test-stable#754 branch from 416d906 to 9f758ad Compare March 16, 2023 11:09
@mateo-moon
Copy link
Contributor Author

mateo-moon commented Mar 16, 2023

This is because tests falling in this group run cargo build and multiple invocation of cargo build lead to clottering execution.

So is it as fast as before the bug occurred? Can you give a short before/after timing please?

I don't have before results. I don't know when "bug" occurred. Mb you could share the link of the invocation in the past so i could compare.
Although i doesn't seem like a bug in nextest to me. I don't actually understand how did it work properly before. But if it's really the case, than i think something has been changed in cargo. Mb with new version update something was changed.
About current set up performance. With one dedicated thread for this group it toll 13 minutes for a whole test run.
I've added new parameter with thread-requiered and set it to 16. It should improve things. Nevermind. This setting is for group of tests not for a separate test. Doesn't help in our case.

@mateo-moon
Copy link
Contributor Author

mateo-moon commented Mar 16, 2023

cargo build and multiple invocation of cargo build lead to clottering execution.

You mean too many parallel execution brought down the CPU?

No, it's not that. Every test by default nextest config is dedicated to one Core. It's more likely because of some kind of the race condition in cargo build. I found this hint in this thread.
I was able to get ui tests properly ran only when they executed sequentially.
Have no idea how it worked before.

@mateo-moon mateo-moon force-pushed the oleg/ci-cd/linux-test-stable#754 branch from 9f758ad to aeed443 Compare March 16, 2023 11:27
 - Add nextest config
 - Add slow tests to mutual exclusion list
@mateo-moon mateo-moon force-pushed the oleg/ci-cd/linux-test-stable#754 branch from aeed443 to fcbe807 Compare March 17, 2023 12:12
Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>
@mateo-moon mateo-moon merged commit 0506da0 into master Mar 17, 2023
@mateo-moon mateo-moon deleted the oleg/ci-cd/linux-test-stable#754 branch March 17, 2023 14:14
@bkchr
Copy link
Member

bkchr commented Mar 17, 2023

cargo build and multiple invocation of cargo build lead to clottering execution.

You mean too many parallel execution brought down the CPU?

No, it's not that. Every test by default nextest config is dedicated to one Core. It's more likely because of some kind of the race condition in cargo build. I found this hint in this thread. I was able to get ui tests properly ran only when they executed sequentially. Have no idea how it worked before.

Interesting! Maybe some issue in Cargo itself.

breathx pushed a commit to gear-tech/substrate that referenced this pull request Apr 22, 2023
* ci-cd#754:Fix slow unit tests

 - Add nextest config
 - Add slow tests to mutual exclusion list

* Update .config/nextest.toml

Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>

---------

Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* ci-cd#754:Fix slow unit tests

 - Add nextest config
 - Add slow tests to mutual exclusion list

* Update .config/nextest.toml

Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>

---------

Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants