chore: disable tests by default for some heavy lib/bins without UTs #12950
Conversation
Codecov Report
@@ Coverage Diff @@
## main #12950 +/- ##
==========================================
+ Coverage 68.27% 68.32% +0.04%
==========================================
Files 1501 1498 -3
Lines 252750 252654 -96
==========================================
+ Hits 172576 172622 +46
+ Misses 80174 80032 -142
Flags with carried forward coverage won't be shown. Click here to find out more. see 13 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
It seems this saved quite some time in CI 🤔️ (but some tests are lost) https://buildkite.com/risingwavelabs/pull-request/builds/33684#018b41c0-3106-4da6-89a4-e62a6951e083 |
|
after merging main it's the same (1591 tests) |
|
@TennyZhuang @BugenZhao How do you think? |
src/utils/runtime/Cargo.toml
Outdated
| [lib] | ||
| test = false |
There was a problem hiding this comment.
So if future developers add unit tests for the runtime crate, they will silently not be run? I'm not sure if this could be a potential risk. 😕
There was a problem hiding this comment.
When adding new tests, it is a good habit to confirm a test would fail when the implementation is erroneous accordingly. But of course we cannot just rely on good habits. It is always better to be automated.
We actually had similar problems before #10749, or maybe some existing tests always pass (i.e. do not panic) even when the implementation violates what the test intends to cover, because the test itself is buggy.
Just sharing some thoughts. Not supporting or opposing the approach here.
There was a problem hiding this comment.
I now have a stupid tool 🤡 https://github.com/xxchan/cargo-utests/blob/main/src/main.rs
It's precise but slow (need to compile to know whether there are tests...) so we can't put it in CI. Alternatively, we can grep #[test] and mark test=false more conservatively. In this case, we can't disable binary targets which should be slow to compile.
There was a problem hiding this comment.
Oh, if the project's layout is very standard. (no autobins=false, all binaries are simple single-file wrappers), I think we can do better.
There was a problem hiding this comment.
Updated with only a few crates. They are heavy and probably won't have tests. Maybe we can start with this. 🤡 @BugenZhao
|
Yes. I might need to come up with some ways to check whether a crate
contains tests but marked as tests=false.
But anyway, I think these crates will hardly contain any “critical” tests.
Mostly just testing some utils.
…On Tue, 24 Oct 2023 at 11:57, Bugen Zhao ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/utils/runtime/Cargo.toml
<#12950 (comment)>
:
> +[lib]
+test = false
So if future developers add unit tests for the runtime crate, they will
silently not be run? I'm not sure if this could be a potential risk. 😕
—
Reply to this email directly, view it on GitHub
<#12950 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJBQZNOY5VOBDIKLAWH5YMDYA44C5AVCNFSM6AAAAAA6FMERIKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMOJTHE4TONBZG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
BTW, maybe we should also similarly set
```
[lib]
bench = false
```
... which would be even slower to compile if the target is not specified
correctly 🤡
|
xxchan
changed the title
|
|
|
too many timeouts
|
|
It seems this is quite useful. But we are again hitting the limit soon... 
|
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
💦💦💦
So that we can avoid them being compiled twice when running unit tests.
No tests are ignored after the change (still 1575 tests, but 84 binaries -> 67 binaries)
Note: there aren't significant improvements, because the bottleneck is not improved.
ref: #9878 (comment)
Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.