Extend common_test PropEr with atomlimit-safe generator variants

[Maria-12648430]

The atom generator of PropEr generates atoms from random strings and its use, explicitly or implicitly, is prone to exhausting the atom limit.

This PR adds two variants to the atom generator (called existing_atom and safe_atom) which pick from the existing atoms and do not generate any new ones. It also provides variants of the any (--> safe_any), list (--> safe_list), and tuple (--> safe_tuple) generators which implicitly use the safe_atom generator.

The existing_atom generator simply picks from the set of existing atoms. The safe_atom generator does the same but emphasizes some of the most common atoms, the list of known node names, and some "weird" atoms (non-ASCII, latin1 and UTF-8).

The code for the safe_any generator mimicks the any generator in PropEr and makes use of some PropEr-internal functions in the proper_arith module.

The additions of this PR have been locally tested in the lists_property_test_SUITE and work well there. However, the PR does not include those changes, as they are better suited in a seperate PR. OTOH, the absence of a use case probably makes it difficult to review...

[github-actions]

CT Test Results

    1 files    11 suites   4m 32s ⏱️
  93 tests   91 ✔️ 2 💤 0 ❌
109 runs  107 ✔️ 2 💤 0 ❌

Results for commit c39caa3.

♻️ This comment has been updated with latest results.

[Maria-12648430]

Hm, ok, there is a problem... The ct_property_test_proper_ext module contains calls to proper_arith and proper_types, which are unknown unless PropEr is actually present. This (naturally) causes warnings in Dialyzer, and some other tests which check for undefined functions in OTP to fail.

In a sense, the calls to maybe-unknown functions was on purpose: they would magically pop into existence when PropEr was used, and if not, the include of the respective file would just not happen. For the same reason, none of the PropEr macros like ?LET were not used in there, but their expanded forms like proper_types:bind/3.

Is there a better way to do this? Please advise 🥺

[bjorng]

I suggest that you place at least the BEAM file for ct_property_test_proper_ext into a new directory (or into the existing priv directory). Then update ct_property_test:init_per_suite/1 to also add the directory for ct_property_test_proper_ext to the code path. (Perhaps the module should also be renamed to property_test_proper_ext.)

[Maria-12648430]

Thanks @bjorng 🙂

[jhogberg]

Wouldn't it be better to fix PropEr instead? We're not the only ones to have this problem and it feels rather strange to call internals of a 3rd-party library in tests.

[Maria-12648430]

@jhogberg yes...

• We brought the issue up a year ago (Atom generator exhausts the atom limit proper-testing/proper#290) and basically got told that limiting the generators to the existing atoms (or any other way) was not the PropEr solution and that Erlang should implement atom garbage collection instead.
• We tried solutions that add atom limiting ability rather than replacing the current generator entirely: (Add ability to limit the number of generated atoms proper-testing/proper#307 (retracted) and Add generator for existing atoms proper-testing/proper#310 (stalled)). We couldn't get the tests working, though, they're very complex and I don't understand half of what most of them do 🤯

OTOH, I have been adding a few property test suites to OTP, last of them in #7256, which make heavy use of the any and thereby the atom generator, and the point where we can't add any more property tests to OTP because of this is drawing closer. I had to go for some trickery in the lists proptest suite already, which by itself exhausted the atom limit halfway through.

[max-au]

Isn't it a bit of a layering violation, making OTP source code aware of PropEr? Appears to be a sort of a circular dependency. I do support @jhogberg suggestion to try fitting it into PropEr, rather than OTP, as it also allows usages from eunit or other test runners that do not depend on Common Test.

[bjorng]

Isn't it a bit of a layering violation, making OTP source code aware of PropEr? Appears to be a sort of a circular dependency.

It is an optional dependency, not even mandatory for testing OTP. Basic functionality should always be tested with "plain" test suites.

Since the maintainers of PropER won't accept the updated generators, I think this is the best we can do.

One thing regarding layering, the property-based tests are supposed to work with any of the known property-based tools, so if another tool than PropER is used, it would be nice if the new generators would fall back to calling the standard generators in that tool.

[Maria-12648430]

Since the maintainers of PropER won't accept the updated generators, I think this is the best we can do.

I wouldn't go as far as unconditional "won't accept", but it looks like it's very difficult to come up with something that they will accept...

One thing regarding layering, the property-based tests are supposed to work with any of the known property-based tools, so if another tool than PropER is used, it would be nice if the new generators would fall back to calling the standard generators in that tool.

Ah well, as to that... 😅 At least the proptests I wrote for OTP in the past (for lists, base64, queue, ...) in fact do depend on PropEr and won't work with the others, as some of the generators used there are present in PropEr only. Notably, there are no atom/0 and any/0 generators in EQC or Triq, and there are no list/0 or tuple/0 generators in them, either, but they are used in those proptests a lot.

If the goal is that the proptests in OTP work with all three tools, this PR is actually obsolete: the atom generator is not present in all of them but only PropEr, so we can't use it, so we don't need a safe version of it as far as OTP proptests are concerned. And the existing PropEr-dependent tests need to be reworked (not sure how much boilerplate code that would require and how much "steam" it would take out of those tests).

[Maria-12648430]

Wait... Triq does have atom and any generators... They work pretty much the same way as PropEr's do. EQC doesn't have them, though.

Anyway, looking over all the tools, it looks like staying on the common ground is pretty limiting overall... And some of the things they do share work in somewhat different ways.

[bjorng]

@Maria-12648430 It is no longer a goal that property tests should work in all of them. We use PropER exclusively in our daily builds. There is no need trying to maintain compatibility with EQC and Triq.

[Maria-12648430]

@bjorng great, that's one worry less 🤗

[Maria-12648430]

So, @bjorng, I take this as a "go ahead" then? 😉

To be sure, the intention here is to use these new generators in OTP-internal tests only, ie they're not for the general public.

In the future, PropEr may accept my PR for the same issue, one way or other. In that case, we may have clashing existing_atom() generator functions. For that reason, I'm thinking of not importing them (as I do in the current state of this PR) but require an explicit call, so the module may have to be renamed to something more wieldy. If that (my PropEr PR being accepted) ever happens, the OTP proptests will keep working and can be adapted to the future PropEr release as time permits.

That said, what is OTPs rule in respect to what PropEr releases to use? The latest? A fixed one?

[bjorng]

Yes, it is a go ahead!

Requiring an explicit external call is a good idea to make it clear that it is not a standard PropER feature.

At least on some of the build machines we only update PropEr when there is a new feature we'll need. I have just updated PropER to the latest master commit while rebuilding it because of #7396 (before that, we used 1.04, the latest tagged release). I am not sure exactly how PropEr is updated for our Docker builds. If there is some new feature in PropER that you'll need for the property test, just tell us and we'll make sure that it used on all our test machines.

[Maria-12648430]

Ok, great, then I will pick this up again in the next few days 🙂

[Maria-12648430]

The last commits remove the calls to PropEr-internal functions in the proper_arith module and replaces them with custom-tailored and streamlined implementations. Thus, if PropEr internals change, the custom generators are not affected.

Also, the importing of the new custom generators has been removed again.

[Maria-12648430]

@bjorng

I suggest that you place at least the BEAM file for ct_property_test_proper_ext into a new directory (or into the existing priv directory).

Umm... how do I go about that? 😅 I guess it means fiddling around in the Makefiles, something which is not exactly at the top of my list of strong points 🙃

[bjorng]

how do I go about that?

@Maria-12648430 I can help with that. I'll try to get to it next week.

[Maria-12648430]

@Maria-12648430 I can help with that. I'll try to get to it next week.

Thanks, that would be great 🤗

[Maria-12648430]

I have just updated PropER to the latest master commit while rebuilding it because of #7396 (before that, we used 1.04, the latest tagged release).

@bjorng by "just", I assume you mean less than ~8 months ago, later than October 27th, 2022, yes? That was when the map generator was added to PropEr master, so I would add a safe_map generator to this module.

[Maria-12648430]

The last commits fix a bug in the safe_tuple generator, adjust the type frequencies in the safe_any generator (they were really different from PropEr's), and add a safe_map generator.

I'm still at a loss regarding a handy module name, open for suggestions 😅

[bjorng]

I have just updated PropER to the latest master commit while rebuilding it because of #7396 (before that, we used 1.04, the latest tagged release).

@bjorng by "just", I assume you mean less than ~8 months ago, later than October 27th, 2022, yes? That was when the map generator was added to PropEr master, so I would add a safe_map generator to this module.

Yes, I did the update the same day that I wrote the comment.

[bjorng]

This week has been busy for me, so I'll do the Makefile changes next week.

I will not work tomorrow. Tomorrow is Midsummer Eve in Sweden. I will be back on Monday.

[Maria-12648430]

This week has been busy for me, so I'll do the Makefile changes next week.

Sure, no hurry ☺️

I will not work tomorrow. Tomorrow is Midsummer Eve in Sweden. I will be back on Monday.

Enjoy your holiday (which we here in Germany know mainly by ways of IKEA 😅)

[bjorng]

I have pushed a new commit that renames your extension module to proper_ext and moves it into the directory lib/common_test/proper_ext.

[juhlig]

I have pushed a new commit that renames your extension module to proper_ext and moves it into the directory lib/common_test/proper_ext.

I have tried it out with the property tests for the queue module and confirm that it works as expected 👍 I noticed that proper has an alias named term for the any type, so there should also be an alias named safe_term for the safe_any type in proper_ext.

The name proper_ext may be a little problematic, though, as the modules in PropEr itself are by convention all prefixed with proper_*. So if PropEr ever introduces a module proper_ext, even if just for internal purposes, there will be a clash and OTP will have to adapt by renaming, including all the property tests that make use of it. How about naming it ct_proper_ext instead?

[Maria-12648430]

Nice, works great, thanks @bjorng 🤗

[bjorng]

The name proper_ext may be a little problematic, though, as the modules in PropEr itself are by convention all prefixed with proper_*. So if PropEr ever introduces a module proper_ext, even if just for internal purposes, there will be a clash and OTP will have to adapt by renaming, including all the property tests that make use of it. How about naming it ct_proper_ext instead?

Yes, that's probably better. @Maria-12648430, will you do the renaming? When done, please squash the commits.

[Maria-12648430]

Yes, that's probably better. @Maria-12648430, will you do the renaming? When done, please squash the commits.

On it 👍 Should I rename only the module or the directory as well?
I'm also putting in some comments while I'm at it.

When done, please squash the commits.

Will do 🙂

[bjorng]

I see no reason to rename the directory.

[Maria-12648430]

@bjorng all done 😃

[bjorng]

Thanks! Added to our daily builds.

[bjorng]

Thanks for your pull request.

[Maria-12648430]

Yay 😃