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

Tests don't get properly added to the totals when ASSUME is outside of GIVEN #2836

Open
AsparagusEduardo opened this issue Mar 17, 2023 · 10 comments
Labels
category: battle-tests Related to the automated test environment

Comments

@AsparagusEduardo
Copy link
Collaborator

This might just be a thing of improperly made tests.

image
image

@mrgriffin
Copy link
Collaborator

mrgriffin commented Mar 27, 2023

This is possibly due to the first pass over the test which counts how many PARAMETRIZEs there are. In hindsight we could probably remove this pass and reallocate a bigger results array before calling into the test if needed.

EDIT: Actually my suggestion isn't great, because it would make it difficult to compute the cost of our test cases. But on the other hand, with the structured RNG having landed the costs are much more uniform now.

@mrgriffin mrgriffin added bug Bug category: battle-mechanic Pertains to battle mechanics new-feature Adds a feature category: battle-tests Related to the automated test environment and removed bug Bug category: battle-mechanic Pertains to battle mechanics new-feature Adds a feature labels Mar 27, 2023
@mrgriffin
Copy link
Collaborator

This is possibly (hopefully! 🤞) improved by the estimateCost changes in #2955.

@hedara90
Copy link
Collaborator

Is this issue still relevant or could it be restated?
It still seems to be a problem with ASSUME usage outside of ASSUMPTIONS and GIVEN, but in a different way.
20240822_09h38m18s_grim
When ASSUME is placed outside of GIVEN in a single test it generates one "ASSUME failed" per thread.
Would it be possible to make it so that ASSUMES places outside of GIVEN/ASSUMPTIONS fail to compile?

@AlexOn1ine
Copy link
Collaborator

@AsparagusEduardo this should be fixed by now, right?

@AsparagusEduardo
Copy link
Collaborator Author

@AsparagusEduardo this should be fixed by now, right?

it is as @hedara90 said, still not fixed.
image

Would it be possible to make it so that ASSUMES places outside of GIVEN/ASSUMPTIONS fail to compile?

Either that or mark them as INVALID like other tests syntaxes are made

@AsparagusEduardo
Copy link
Collaborator Author

AsparagusEduardo commented Aug 31, 2024

Would it be possible to make it so that ASSUMES places outside of GIVEN/ASSUMPTIONS fail to compile?
Either that or mark them as INVALID like other tests syntaxes are made

I don't know how to do this though. I can make a PR that makes all ASSUMES be inside of GIVEN and we start looking for it in reviews?
EDIT: In the documentation, we already have ASSUMEs inside GIVEN

@hedara90
Copy link
Collaborator

hedara90 commented Sep 1, 2024

From what I saw when I looked through the code there was only 1 ASSUME that was outside ASSUMPTIONs or GIVEN blocks (in test/random.c).

@AsparagusEduardo
Copy link
Collaborator Author

From what I saw when I looked through the code there was only 1 ASSUME that was outside ASSUMPTIONs or GIVEN blocks (in test/random.c).

I have the brach ready, there are a lot more (over 30).

@hedara90
Copy link
Collaborator

hedara90 commented Sep 1, 2024

Ah, I kinda meant "that have to be outside"

@AsparagusEduardo
Copy link
Collaborator Author

#5308

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: battle-tests Related to the automated test environment
Projects
None yet
Development

No branches or pull requests

4 participants