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.

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

Use generator for existing atoms in lists property tests #7287

Conversation

Maria-12648430
Copy link
Contributor

The atom() generator of PropEr creates atoms from random strings, and is thereby prone to exhaust the atom limit. By extension, so are the generators containing the atom() generator, such as any() or list().

The lists property test suite exhausted the atom limit at around 80 (of 107) properties, and we had to find a workaround by pre-generating a limited set of random atoms, store them in persistent_term, and a generator that subsequently took atoms only from this set instead of generating new ones.

This PR changes the custom atom generator again to not pre-generate anything but pick from the set of already existing atoms. This has the following benefits over the current version:

  • no need to generate and store (and remember to delete) anything, as everything is already there
  • by using existing atoms, there is a greater likelihood of hitting anything than by simply random ones, as they at least have been written down at least somewhere

Unfortunately, there is no official way of getting at the existing atoms, so this has to rely on an obscure "trick" that somebody figured out at some point. For OTP-internal tests like this one, I think it is justifiable, though.

@github-actions
Copy link
Contributor

github-actions bot commented May 24, 2023

CT Test Results

       2 files       89 suites   42m 19s ⏱️
1 907 tests 1 858 ✔️ 48 💤 1
2 201 runs  2 150 ✔️ 50 💤 1

For more details on these failures, see this check.

Results for commit da5a69b.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

Copy link
Contributor

@RobinMorisset RobinMorisset left a comment

Choose a reason for hiding this comment

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

It generally looks good, but I'd recommend keeping a few hardcoded possible atoms, covering things like "non-ascii but latin1", "non-latin1 but valid utf8", etc..
As long as it is a small finite set it won't cause exhaustion of the atom table, and it will make sure that special code-paths for handling of non-ascii characters are exercised.
Adding such special atoms in erlfuzz helped find things like #7153.

@Maria-12648430
Copy link
Contributor Author

@RobinMorisset

f(X = '原子') ->
   <<X/utf8>>.

'原子', nice pun 🤣

Anyway, this is local to tests for the lists module, where the actual "appearance" of the atoms is of no importance whatsoever, as long as matching/comparison of atoms itself works. And if it doesn't, all hell would break loose all over the place, anyway 🔥

That said, one day I will try to do an extension to the property-based testing facility of common_test to enable it to work in an atom-limit-safe manner. Or so I keep telling myself each time I do some property testing suite for OTP and get reminded of it 😅 (A PR I made in PropEr itself for that issue got somewhat stuck/stalled --> not coming anytime soon it seems 🤷‍♀️). When I get round to it, I will do as you suggest 🙂

@RobinMorisset
Copy link
Contributor

Oh, I had not realized that this is only for the list module.
Thanks for the explanation, your solution seems perfect then.

@bjorng
Copy link
Contributor

bjorng commented May 26, 2023

I don't mind the trick for accessing existing atoms, but I only want to see it in one place in the test suite for STDLIB. That is, if you plan to use the same generator for other tests, please place in its own module that can be shared between property tests for STDLIB.

@bjorng bjorng added the team:VM Assigned to OTP team VM label May 26, 2023
@bjorng bjorng self-assigned this May 26, 2023
@Maria-12648430
Copy link
Contributor Author

If #7364 is accepted, this PR is obsolete. In that case, a new PR should be created to make use of the new generators.

@Maria-12648430
Copy link
Contributor Author

Obsoleted by #7364, will make another PR that uses ct_proper_ext.

@Maria-12648430
Copy link
Contributor Author

Superseded by #7454.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants