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

ci: Set the "render" kind on tests that need renderer, mark them as heavy for nextest #17574

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

ci: Set the "render" kind on tests that need renderer, mark them as heavy for nextest #17574

wants to merge 5 commits into from

Conversation

torokati44
Copy link
Member

@torokati44 torokati44 commented Aug 19, 2024 •
edited
Loading

But it doesn't seem to work locally for me...? 😵‍💫

Turns out, updating cargo-nextest fixed it for me...

Relevant docs pages:
https://nexte.st/docs/configuration/threads-required/
https://nexte.st/docs/filtersets/reference/

This will hopefully stop random crashes of visual tests (especially on avm2/pixelbender_shaderdata and avm2/graphics_round_rects) on GHA.

@torokati44 torokati44 marked this pull request as ready for review August 19, 2024 19:26
@torokati44 torokati44 force-pushed the heavy-render-tests branch 2 times, most recently from bbf45b4 to 1049813 Compare August 19, 2024 19:28
@torokati44 torokati44 added A-tests Area: Tests & Test Framework github_actions Pull requests that update GitHub Actions code waiting-on-review Waiting on review from a Ruffle team member labels Aug 19, 2024
@Dinnerbone
Copy link
Contributor

I think I'd prefer a suffix, usually things are module::path::testname, so this kinda fits

@torokati44
Copy link
Member Author

Yeah, that's what I tried first, but...
image

It kinda makes render look like it's the test name, and the test name like it's something else...?

@Dinnerbone
Copy link
Contributor

Hmmm okay. Prefix looks fine then

@torokati44
Copy link
Member Author

Cool! Does this count as a green checkmark? 🤔

kjarosh
kjarosh requested changes Aug 20, 2024
View reviewed changes
Copy link
Member

@kjarosh kjarosh left a comment

Choose a reason for hiding this comment

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

Unfortunately this will not work properly. There is a certain contract we have to adhere to when using libtest_mimic — if we list a test, we should also be able to find it by its name. In case of nextest, tests which do not comply with this requirement will not raise any errors and will be skipped and marked as passed.

Compare logs from this PR and from master. The test visual/shumway_acid_tests/acid_big took ~0.005s instead of ~1.889s.

I would personally abstain from playing with test names (it's very easy to skip some tests by accident that way), and try using test kinds for that purpose, as we already have support for them (and tests with a kind are being looked up properly).

@torokati44
Copy link
Member Author

Good catch, thanks!

Can a test have any number of kinds though?

@kjarosh
Copy link
Member

kjarosh commented Aug 20, 2024

Can a test have any number of kinds though?

A test may have one kind only, but we can join multiple kinds with a comma (I think, not entirely sure it'll work) and add support for multiple kinds that way ourselves.

@kjarosh
Copy link
Member

kjarosh commented Aug 20, 2024

Now thinking about it... a test kind is just a string prefix (but inside brackets), which is ignored when looking up tests. I think we can safely put multiple strings separated by a comma there and it'll work, even if we pass no kind or an invalid kind when looking up tests.

The only situation when the contents of a kind matter is when we parse them, so in .config/nextest.toml and possibly in the code.

@kjarosh kjarosh added waiting-on-author Waiting on the PR author to make the requested changes and removed waiting-on-review Waiting on review from a Ruffle team member labels Aug 20, 2024
@torokati44
Copy link
Member Author

Okay, stole some bits and pieces from #15950, now render is a kind, not a name prefix.

@torokati44 torokati44 changed the title ci: Prefix test names that need renderer with "render::" make them heavy for nextest ci: Set the "render" kind on tests that need renderer, mark them as heavy for nextest Aug 20, 2024
kjarosh
kjarosh reviewed Aug 20, 2024
View reviewed changes
.config/nextest.toml Outdated
[[profile.ci.overrides]]
filter = "test(avm2/pixelbender_shaderdata)"
retries = 3
filter = "kind(render)"
Copy link
Member

Choose a reason for hiding this comment

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

I believe the nextest's kind is something different from the libtest_mimic's kind. See https://nexte.st/docs/filtersets/reference/#binary-kinds

Copy link
Member

Choose a reason for hiding this comment

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

I think you'd have to use a regular expression with test(...), e.g. test(/\[render\] .*/)

Copy link
Member

Choose a reason for hiding this comment

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

Or rather: test(/^\[render\] /)

Copy link
Member Author

@torokati44 torokati44 Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

How about a glob pattern? like test(#[render] *). Less escaping!
EDIT: Hmmm, it doesn't work...

@torokati44 torokati44 force-pushed the heavy-render-tests branch 3 times, most recently from f9ae076 to 007df30 Compare August 20, 2024 16:46
@torokati44
Copy link
Member Author

torokati44 commented Aug 20, 2024 •
edited
Loading

Aaaand now it automatically does the [render] tests in one group first, so it makes the failure more likely... Let's try a weight of 3, so no two render tests can run at the same time, but other ones can together with them...?

@torokati44
Copy link
Member Author

Even one at a time they crash... 😨😵😵‍💫🫥🫠

@kjarosh
Copy link
Member

kjarosh commented Aug 20, 2024

Maybe just set a higher number of retries for render tests for now?

@torokati44
Copy link
Member Author

Maybe just set a higher number of retries for render tests for now?

Interestingly, it's again only the same two tests that crashed. So I'd allow retries only for those then - but then we're almost back to square one.

@torokati44 torokati44 marked this pull request as draft August 20, 2024 18:04
@torokati44 torokati44 mentioned this pull request Aug 20, 2024
Merged
@torokati44 torokati44 force-pushed the heavy-render-tests branch 2 times, most recently from 8010d24 to 0b9f160 Compare August 21, 2024 09:56
@torokati44
Copy link
Member Author

torokati44 commented Aug 21, 2024 •
edited
Loading

Finally, a stack trace!

https://github.com/ruffle-rs/ruffle/actions/runs/10488227410/job/29050242404?pr=17574#step:8:7584

Thread 11 (Thread 0x7fffc7e006c0 (LWP 14650) "tests-6cab02727"):
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=<optimized out>, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x00007ffff7c4526e in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x00007ffff7c288ff in __GI_abort () at ./stdlib/abort.c:79
#5  0x00007ffff7c297b6 in __libc_message_impl (fmt=fmt@entry=0x7ffff7dce8d7 "%s\n") at ../sysdeps/posix/libc_fatal.c:132
#6  0x00007ffff7ca8fe5 in malloc_printerr (str=str@entry=0x7ffff7dd1ae0 "double free or corruption (!prev)") at ./malloc/malloc.c:5772
#7  0x00007ffff7cab11c in _int_free_merge_chunk (av=0x7ffff7e03ac0 <main_arena>, p=0x5555575a0020, size=1040) at ./malloc/malloc.c:4679
#8  0x00007ffff7cab42a in _int_free (av=0x7ffff7e03ac0 <main_arena>, p=<optimized out>, have_lock=<optimized out>) at ./malloc/malloc.c:4646
#9  0x00007ffff7cadd9e in __GI___libc_free (mem=0x5555575a0030) at ./malloc/malloc.c:3398
#10 0x00007fffeee63f17 in void std::vector<std::unique_ptr<llvm::PassInfo const, std::default_delete<llvm::PassInfo const> >, std::allocator<std::unique_ptr<llvm::PassInfo const, std::default_delete<llvm::PassInfo const> > > >::_M_realloc_insert<std::unique_ptr<llvm::PassInfo const, std::default_delete<llvm::PassInfo const> > >(__gnu_cxx::__normal_iterator<std::unique_ptr<llvm::PassInfo const, std::default_delete<llvm::PassInfo const> >*, std::vector<std::unique_ptr<llvm::PassInfo const, std::default_delete<llvm::PassInfo const> >, std::allocator<std::unique_ptr<llvm::PassInfo const, std::default_delete<llvm::PassInfo const> > > > >, std::unique_ptr<llvm::PassInfo const, std::default_delete<llvm::PassInfo const> >&&) () from /lib/x86_64-linux-gnu/libLLVM-17.so.1
#11 0x00007fffeee63443 in llvm::PassRegistry::registerPass(llvm::PassInfo const&, bool) () from /lib/x86_64-linux-gnu/libLLVM-17.so.1
#12 0x00007fffeef94ec7 in ?? () from /lib/x86_64-linux-gnu/libLLVM-17.so.1
#13 0x00007ffff7ca1ec3 in __pthread_once_slow (once_control=0x7ffff55fc194, init_routine=0x7ffff64e93b0 <__once_proxy>) at ./nptl/pthread_once.c:116
#14 0x00007fffeef94ded in llvm::initializeEarlyIfConverterPass(llvm::PassRegistry&) () from /lib/x86_64-linux-gnu/libLLVM-17.so.1
#15 0x00007fffeef40042 in llvm::initializeCodeGen(llvm::PassRegistry&) () from /lib/x86_64-linux-gnu/libLLVM-17.so.1
#16 0x00007fffef2c3dec in llvm::TargetPassConfig::TargetPassConfig(llvm::LLVMTargetMachine&, llvm::legacy::PassManagerBase&) () from /lib/x86_64-linux-gnu/libLLVM-17.so.1
#17 0x00007ffff1a1727f in ?? () from /lib/x86_64-linux-gnu/libLLVM-17.so.1
#18 0x00007fffef030d5a in llvm::LLVMTargetMachine::addPassesToEmitMC(llvm::legacy::PassManagerBase&, llvm::MCContext*&, llvm::raw_pwrite_stream&, bool) () from /lib/x86_64-linux-gnu/libLLVM-17.so.1
#19 0x00007ffff0837ac9 in llvm::MCJIT::emitObject(llvm::Module*) () from /lib/x86_64-linux-gnu/libLLVM-17.so.1
#20 0x00007ffff0837c6d in llvm::MCJIT::generateCodeForModule(llvm::Module*) () from /lib/x86_64-linux-gnu/libLLVM-17.so.1
#21 0x00007ffff08386ed in llvm::MCJIT::finalizeObject() () from /lib/x86_64-linux-gnu/libLLVM-17.so.1
#22 0x00007ffff07bf287 in LLVMGetPointerToGlobal () from /lib/x86_64-linux-gnu/libLLVM-17.so.1

"double free or corruption (!prev)")

Hmm, LLVM bug?

@torokati44 torokati44 mentioned this pull request Aug 21, 2024
Open
@torokati44
Copy link
Member Author

Cross-referencing, about the crash: https://gitlab.freedesktop.org/mesa/mesa/-/issues/11797

@danielhjacobs danielhjacobs added the T-chore Type: Chore (like updating a dependency, it's gotta be done) label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kjarosh kjarosh kjarosh requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
A-tests Area: Tests & Test Framework github_actions Pull requests that update GitHub Actions code T-chore Type: Chore (like updating a dependency, it's gotta be done) waiting-on-author Waiting on the PR author to make the requested changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@torokati44 @Dinnerbone @kjarosh @danielhjacobs