-
Notifications
You must be signed in to change notification settings - Fork 16
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
Weak test revisions #365
Weak test revisions #365
Conversation
- to use easier to shrink int64s - doing a full major GC to have a clean starting point - doing a minor GC in between attempts - increasing the frequency of cmds that make us observe the state
- also adding minor and major GC calls for reproducability - increasing the frequency of state observing cmds - adding a simple cmd shrinker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good! 😄
The only thing I would have done differently is the support for 5.0. I’ve used this in the past: shym/ortac@cecf0c5 which has the nice side-effect that it doesn’t override the standard module when not needed. But I’m not sure this makes such a difference.
Good point about overriding 👍 |
Nice trick 👍 |
The shadowing is handled in 9b85211. |
CI summary for 720028c
Modulo the first one, none of this is related to this PR. |
Small suggestion for the last commit: rather than disabling the warning globally, it can be disabled only where it might trigger, it’s easier to understand why it is required. @@ -41,6 +41,7 @@ struct
module Int64 =
struct
+ [@@@warning "-unused-value-declaration"]
(* support Int64.hash added in 5.1 *)
external seeded_hash_param :
int -> int -> int -> 'a -> int = "caml_hash" [@@noalloc] |
CI summary of latest run
|
d45824f
to
f3d0160
Compare
Fair enough. No need to silence more than necessary. Should be addressed in f3d0160 👍 |
CI summary for the latest run:
I'll go ahead and merge. |
This PR revises our
Weak
tests...The
Weak
module depends on the state of the GC.As such, we have to be extra careful to start from "as clean a starting point as possible".
Just having run sequential tests pollutes the heap state - so better run a
Gc.full_major ()
initially - and a cheaperGc.minor ()
in between attempts.Secondly, the PR switches away from
string
s to more easy-to-shrinkint64
s.Finally I think running
Lin
tests ofWeak
is misguided, as I don't think it makes sense to perform a fragile GC-dependent parallel run and compare its outputs against more fragile GC-dependent sequential runs.Parallel
Weak
STM
tests however work well as stress tests - we previously caught run-time bugs that way (e.g., #181)Hopefully this will solve #299.
I can see a 5.2 s390x run even completed within the timelimit with this: 🎉
https://ocaml-multicoretests.ci.dev:8100/job/2023-06-09/150708-ci-ocluster-build-44e4ee